Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Aug 23, 2021

Closes #1326

This simplifies things by removing .set field. It also fixes an inference issue in keys.

The downside is that it breaks the iteration order for items in a CleverDict initialized with storage but not yet filled.

To be honest, I don't really understand the need for pre-initializing the CleverDict. I've left the option, but it now calls size hint instead of pre-allocating the space.

@odow odow added the Submodule: Utilities About the Utilities submodule label Aug 23, 2021
@odow odow requested review from joaquimg and blegat August 23, 2021 05:59
@blegat
Copy link
Member

blegat commented Aug 23, 2021

The downside is that it breaks the iteration order for items in a CleverDict initialized with storage but not yet filled.

Can you elaborate on that ? I don't understand the change of iteration order

@odow
Copy link
Member Author

odow commented Aug 23, 2021

d = CleverDicts.CleverDict{Int,Float64}(hash, inverse_hash, 3)
d[4] = 0.25
d[2] = 1.5

used to iterate 2, 4 because it assumed there were three elements, then we added d[4], then set d[2]. That leaves 1 and 3 as undef.

Now preallocating with 3 will sizehint!(d.vector, 3), but not actually preallocate the storage.

@blegat
Copy link
Member

blegat commented Aug 23, 2021

Ok I see, the OrderedDict iterates by order of creation while the vector storage iterates by order 1:n. There order match if we only allow the vector storage to be filled in the order 1, 2, 3, ....
In your example, we were using the vector storage even if it wasn't filled in the right order. That makes the order of iteration not match the order of creation which wasn't great. I would say this changes is an improvement.

@odow
Copy link
Member Author

odow commented Aug 23, 2021

I'll wait for @joaquimg to take a look before merging.

@joaquimg
Copy link
Member

I am ok with removing the sizehint on init. People can do that in two steps.

function CleverDict{K,V}(
hash::F,
inverse_hash::I,
n::Integer = 0,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaquimg says we should just remove this argument.

@odow odow merged commit cd51169 into master Aug 26, 2021
@odow odow deleted the od/cleverdicts branch August 26, 2021 00:03
@odow
Copy link
Member Author

odow commented Aug 26, 2021

I'll remove the input argument in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Submodule: Utilities About the Utilities submodule
Development

Successfully merging this pull request may close these issues.

set field of CleverDict
3 participants