Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix length of FrozenDict #10

Merged
merged 3 commits into from
Apr 19, 2021
Merged

fix length of FrozenDict #10

merged 3 commits into from
Apr 19, 2021

Conversation

ilia-kats
Copy link
Contributor

We need a functioning length() to get the iterators to behave correctly. Since all arrays inside FrozenDict are potentially larger than the number of elements in the dict, we need to store the length of the original array.

This change was already introduced in my previous PR and later reverted in 2b981b9. If you object to altering the meaning of the sz variable, we can also add a new element to the FrozenDict struct specifically for storing the number of elements.

ilia-kats added a commit to scverse/Muon.jl that referenced this pull request Apr 19, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2021

Codecov Report

Merging #10 (5df4637) into master (50c51d6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #10   +/-   ##
=======================================
  Coverage   58.40%   58.40%           
=======================================
  Files           1        1           
  Lines         125      125           
=======================================
  Hits           73       73           
  Misses         52       52           
Impacted Files Coverage Δ
src/CompressHashDisplace.jl 58.40% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50c51d6...5df4637. Read the comment docs.

@Arkoniak
Copy link
Owner

Ah, yes, I see now.
TBH, I do not like the idea of changing FD.sz to length(FD.ks) because I am not sure that the compiler will always inline this function.

Also, the current implementation of CHD is fragile, since it works only for String and other "big" objects like that. For numerical keys for example, isassigned will always return true. It was not relevant for getindex since the probability that random number equals to key is minuscule, but it is more important for iterate protocol.

How about adding mask field of the length sz, of the type Vector{Bool} which contains positions of the keys?

And if you can add tests that would be awesome, much easier to detect things that shouldn't be changed.

@Arkoniak Arkoniak merged commit 71f6477 into Arkoniak:master Apr 19, 2021
@Arkoniak
Copy link
Owner

Thank you for your work!

If you want to use this package and since I am not actively developing this package, I can give you maintainer privileges for this repo, so you don't have to wait for my peer review.

@ilia-kats
Copy link
Contributor Author

Thanks, but I think there is enough functionality for my usecase at the moment. I will get back to you if it turns out that more functionality is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants