Skip to content

Conversation

joaquimg
Copy link
Member

@joaquimg joaquimg commented Aug 6, 2020

Follows: #1129

This avoids the conversion from dense dict to standard dict in copy functions.

# Conflicts:
#	src/Utilities/cachingoptimizer.jl
#	src/Utilities/dense_dict.jl
@blegat
Copy link
Member

blegat commented Aug 7, 2020

I remember we hesitated between adding a dict in DenseDict or using DenseDict inside a CleverDict to switch from DenseDict to Dict in case of deletion. I thought we decided the second one but I don't remember the details

@blegat
Copy link
Member

blegat commented Aug 7, 2020

I think we should

  1. replace the vector by a DenseDict in CleverDict
  2. add the possibility to add a new contiguous index to DenseDict
  3. implement setindex! for CleverDict similarly to this PR

@joaquimg
Copy link
Member Author

joaquimg commented Aug 7, 2020

I remember we hesitated between adding a dict in DenseDict or using DenseDict inside a CleverDict to switch from DenseDict to Dict in case of deletion. I thought we decided the second one but I don't remember the details

I think we only hesitated because it could be too similar to CleverDict.

@joaquimg
Copy link
Member Author

joaquimg commented Aug 7, 2020

We need to be careful with CleverDict because of the downstream usage.
We need to check if theses changes will be breaking.

@joaquimg
Copy link
Member Author

joaquimg commented Aug 7, 2020

one problem is that:
CleverDicts requires the methods index_to_key and key_to_index

function index_to_key(::Type{MathOptInterface.VariableIndex}, index::Int)

As methods to be overloaded, while DenseDicts get hash and inverse_hash in the type constructor.
This would be breaking for solvers like GLPK that overload the CleverDict methods:

https://github.com/jump-dev/GLPK.jl/blob/41e649cd81864e90065ac78f42317edcebcd9951/src/MOI_wrapper/MOI_wrapper.jl#L34

@blegat
Copy link
Member

blegat commented Aug 8, 2020

We could use these functions as default for the map and inverse map of DenseDict so that it is non-breaking

@odow
Copy link
Member

odow commented Aug 8, 2020

It is a bit confusing that we have so many types of dictionaries. It's probably okay to get some performance improvement for now, but at some point I'd like to go through and tidy up a lot of this.

@joaquimg
Copy link
Member Author

joaquimg commented Aug 8, 2020

So instead of adding a DenseDict inside CleverDict I will merge the two of them.
As @blegat that can be done without breaking. And DenseDict was never released anyway.

@blegat
Copy link
Member

blegat commented Aug 8, 2020

Merging them sounds good

@blegat
Copy link
Member

blegat commented Aug 30, 2020

@joaquimg Is this closed by #1142 ?

@odow odow closed this Sep 2, 2020
@odow odow deleted the jg/grow_densedict branch September 2, 2020 03:48
@odow odow restored the jg/grow_densedict branch September 2, 2020 03:48
@odow odow deleted the jg/grow_densedict branch September 2, 2020 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants