Refactor DoubleDicts.jl #1446

No there are plan to us it in more places than the IndexMap, e.g. UniversalFallback, DiffOpt but it hasn't been done yet.
MOI brings a generic and extensible interface but supporting any function and set types should not mean that all the code become unstable. We should educate the user to store things in DoubleDicts, use function barriers taking type-stable versions of the double dicts through the barrier.
The current DoubleDicts cover many possible use cases but I've encountered cases that are not covered, e.g., where what you store depends on F and S but is not exactly Tuple{F,S}

odow · 2021-07-06T21:06:08Z

but it hasn't been done yet

My problem with this is that we have a lot of code in MOI that is sitting there un-used on the off-chance that it might be useful in the future. We should tear it out prior to 1.0. We can always add it back later...

odow · 2021-07-07T04:53:23Z

Okay. This needs a pretty thorough review by @joaquimg and @blegat.

The changes are actually less scary than the diff seems. It may be best to review in side-by-side mode. The interleaved diff isn't very helpful.

Removed FunctionSetDoubleDict. If we need this later, it can get added back then.
Massively simplify the type signatures of AbstractDoubleDict and AbstractWIthType (now AbstractDoubleDictInner)
Rename WithType to DoubleDictInner
Rename IndexWithType to IndexDoubleDictInner

odow · 2021-07-11T00:10:47Z

Bump @blegat

joaquimg · 2021-07-11T05:18:01Z

Will read tomorrow

joaquimg

The use case for ParametricOptInterface would be ci as keys and arrays of Linear/QuadraricTerms as values.

DiffOpt already uses CIs as keys and functions (SAF) as values.

I am planning to test DDs in xpress in the short term. CleverDicts inside double dicta might be nice to consider.

src/Utilities/DoubleDicts.jl

joaquimg · 2021-07-12T04:47:30Z

Here is one usage of DoubleDicts in DiffOpt:
https://github.com/jump-dev/DiffOpt.jl/blob/1f289b0e7e1a735bfb76f795bbc3ddfeb31da1ce/src/diff_opt.jl#L107

Here we are not using in POI, but we should be:
https://github.com/jump-dev/ParametricOptInterface.jl/blob/b1629f9f93c299e170f9bbf606f6e3e699ebdeba/src/ParametricOptInterface.jl#L33

I should use it here also:
https://github.com/joaquimg/QuadraticToBinary.jl/blob/462e241de47ff40724c056a193cfba2d0fbd07fb/src/moi.jl#L118

odow · 2021-07-12T05:20:24Z

These use-cases are all fine for DoubleDict. I don't think anything needs changing.

joaquimg · 2021-07-12T14:16:43Z

As long as values have a single type they fall into the simpler version of DoubleDict.

The odd one is DiffOpt, which has two DD's, one for scalar and another for vector. It is not clear to me if they should be merged.

joaquimg · 2021-07-12T14:26:55Z

src/Utilities/DoubleDicts.jl

-const AD = AbstractDict

-const DataTypePair = Tuple{DataType,DataType}
+abstract type AbstractDoubleDict{V} <: AbstractDict{MOI.ConstraintIndex,V} end


If we end up with this simple implementation...
We should rename this to ConstraintIndexedDict and reserve DoubleDict for future flexible implementation. Or ConstraintIndexDict or something along those lines.

DoubleDictInner could be SingleTypeConstraintIndexedDict.

IndexDoubleDict would become ConstraintIndexMap...

joaquimg

We can get rid of a few haskey's

src/Utilities/DoubleDicts.jl

blegat · 2021-07-13T01:15:06Z

Yes with DiffOpt, I could just split it in two but in general, it would be nice to have any value type parametrized by F, S. For DiffOpt, it would be F. The user could give a function mapping F, S to the value. So for constraint indices, it is (F, S)-> CI{F, S} and for DiffOpt, it is (F, S)-> F.

odow · 2021-07-13T03:20:30Z

For DiffOpt, it would be F

The right way to do this is for DiffOpt to implement this locally as a subtype of AbstractDoubleDict, show it has a performance benefit, debug, and them make a PR to MOI.

I don't think we should add in this PR.

odow · 2021-07-14T03:10:05Z

Okay. I've addressed the haskey issue. But we still have our naming problem. I think it's fine to keep DoubleDict as the name. It's a submodule within the Utilities submodule, so it's obviously not for wider consumption.

It solves the major issue, and provides a decent fallback for other cases. I'm in favor of merging as-is.

docs/src/submodules/Utilities/overview.md

src/Utilities/DoubleDicts.jl

blegat · 2021-07-14T13:54:12Z

src/Utilities/DoubleDicts.jl

-struct IndexDoubleDict{DI,DO} <: AbstractDoubleDict{DataTypePair,CI,Int64,DI,DO}
-    dict::DO
+struct IndexDoubleDict <: AbstractDoubleDict{MOI.ConstraintIndex}
+    dict::Dict{Tuple{DataType,DataType},Dict{Int64,Int64}}


We should use a CleverDict here. With VectorOfConstraints and MatrixOfConstraints, the constraint indices are now 1:n for scalar constraints so using CleverDict here would speed up copy time for large LP's

Yup, that is why it was previously parametrized by dict.
That is for the second Dict....
And.... we might consider LittleDict for the outer Dict.... (it is from OrderedCollections) leave it as comment?

so using CleverDict here would speed up copy time for large LP's

Benchmark? I struggle to believe that the time looking up the index translation is significant. I'm against adding complexity, even if it is a minor speed improvement.

When doing the benchmarks for MatrixOfConstraints, I noticed creating the IndexMap was taking a significant time.

In a proper model? Or in a microbenchmark?

src/Utilities/DoubleDicts.jl

docs/src/submodules/Utilities/overview.md

odow · 2021-07-22T18:48:15Z

src/Utilities/DoubleDicts.jl

-struct DoubleDict{V,DI,DO} <: AbstractDoubleDict{DataTypePair,V,Int64,DI,DO}
-    dict::DO
+struct DoubleDict{V} <: AbstractDoubleDict{V}
+    dict::Dict{Tuple{DataType,DataType},Dict{Int64,V}}


Add a function _inner_type(F, S) = V and have

Suggested change

dict::Dict{Tuple{DataType,DataType},Dict{Int64,V}}

dict::Dict{Tuple{DataType,DataType},Dict{Int64}}

odow · 2021-07-22T18:50:01Z

src/Utilities/DoubleDicts.jl

 """
-struct DoubleDict{V,DI,DO} <: AbstractDoubleDict{DataTypePair,V,Int64,DI,DO}
-    dict::DO
+struct DoubleDict{V} <: AbstractDoubleDict{V}


Potentially call it ConstraintIndexDict

odow · 2021-07-22T22:02:49Z

@blegat shall I merge this for now, and we can address any other issues in a follow-up PR?

odow added the Submodule: Utilities About the Utilities submodule label Jul 5, 2021

odow requested review from joaquimg and blegat July 5, 2021 07:10

odow changed the title ~~WIP: refactor DoubleDicts.jl~~ Refactor DoubleDicts.jl Jul 8, 2021

joaquimg reviewed Jul 11, 2021

View reviewed changes

src/Utilities/DoubleDicts.jl Show resolved Hide resolved

src/Utilities/DoubleDicts.jl Outdated Show resolved Hide resolved

src/Utilities/DoubleDicts.jl Outdated Show resolved Hide resolved

joaquimg reviewed Jul 12, 2021

View reviewed changes

src/Utilities/DoubleDicts.jl Outdated Show resolved Hide resolved

src/Utilities/DoubleDicts.jl Outdated Show resolved Hide resolved

odow added 8 commits July 14, 2021 12:06

WIP: refactor DoubleDicts.jl

e86f5e4

Large refactoring of DoubleDicts

4f0149b

Fix formatting

6b32ec6

Fix documentation

ceff36b

Tidy iterators

1d2a9b7

Other fixes

e83a0e2

Tweaks

8269413

Fixes

788dd51

odow force-pushed the od/double-dicts branch from f5b92ef to 788dd51 Compare July 14, 2021 00:10

Update DoubleDicts.jl

78037f0

odow mentioned this pull request Jul 14, 2021

Rewrite MOI.Test #1398

Closed

22 tasks

blegat reviewed Jul 14, 2021

View reviewed changes

docs/src/submodules/Utilities/overview.md Show resolved Hide resolved

blegat reviewed Jul 14, 2021

View reviewed changes

src/Utilities/DoubleDicts.jl Show resolved Hide resolved

blegat reviewed Jul 14, 2021

View reviewed changes

src/Utilities/DoubleDicts.jl Outdated Show resolved Hide resolved

More updates

cac4542

blegat reviewed Jul 14, 2021

View reviewed changes

docs/src/submodules/Utilities/overview.md Show resolved Hide resolved

Update overview.md

7b13bd5

odow mentioned this pull request Jul 21, 2021

Migrate tests of Variable bridges #1481

Merged

odow commented Jul 22, 2021

View reviewed changes

blegat approved these changes Jul 23, 2021

View reviewed changes

odow merged commit 9668d19 into master Jul 23, 2021

odow deleted the od/double-dicts branch July 23, 2021 22:21

	dict::Dict{Tuple{DataType,DataType},Dict{Int64,V}}
	dict::Dict{Tuple{DataType,DataType},Dict{Int64}}

Refactor DoubleDicts.jl #1446

Refactor DoubleDicts.jl #1446

Uh oh!

Conversation

odow commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blegat commented Jul 6, 2021

Uh oh!

odow commented Jul 6, 2021

Uh oh!

odow commented Jul 7, 2021

Uh oh!

odow commented Jul 11, 2021

Uh oh!

joaquimg commented Jul 11, 2021

Uh oh!

joaquimg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joaquimg commented Jul 12, 2021

Uh oh!

odow commented Jul 12, 2021

Uh oh!

joaquimg commented Jul 12, 2021

Uh oh!

joaquimg Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

joaquimg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blegat commented Jul 13, 2021

Uh oh!

odow commented Jul 13, 2021

Uh oh!

odow commented Jul 14, 2021

Uh oh!

Uh oh!

Uh oh!

blegat Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joaquimg Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

odow Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

blegat Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

odow Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

odow Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

odow Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

odow commented Jul 22, 2021

Uh oh!

Uh oh!

odow commented Jul 5, 2021 •

edited

Loading

blegat Jul 14, 2021 •

edited

Loading