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 hashing for choicemaps #466

Merged

Conversation

georgematheos
Copy link
Contributor

This adds a method Base.hash(::ChoiceMap) to Gen, so that (e.g.) choicemaps can be used as keys in dictionaries.

Before this change, the following behavior would occur:

julia> d = Dict(); d[choicemap((:x, 1))] = 2;
julia> haskey(d, choicemap((:x, 1)))
false

Per the Julia documentation, since Gen implements a custom method Base.:(==)(::ChoiceMap, ::ChoiceMap), it should also implement a custom hashing method with the property that a::ChoiceMap == b::ChoiceMap implies that hash(a, h) == hash(b, h) for any partial hash h::UInt.

add correct hashing method for choicemaps
@ztangent
Copy link
Member

Thanks for catching this and making this PR! One worry I have is that because get_values_shallow and get_submaps_shallow do not guarantee a fixed ordering of keys, the resulting hashes may differ depending on iteration order. Could you try adding a test case that compares the hashes of choicemap((:a, 1), (:b, 2)) with choicemap((:b, 2), (:a, 1))? And also sort the values before iterating? Thanks!

@ztangent
Copy link
Member

ztangent commented May 20, 2022

After looking at the Julia implementation of hashing for dictionariesit looks like the easiest way to address the lack of guaranteed order is to repeatedly xor the hashes for each value/submap. Note that you should implement it in a similar way to the fixed implementation to prevent many hash collisions! (See the discussion at https://discourse.julialang.org/t/different-dicts-hashing-to-the-same-value/1441/5)

https://github.com/JuliaLang/julia/blob/7bff5cdd0fab8d625e48b3a9bb4e94286f2ba18c/base/abstractdict.jl#L530-L537

@georgematheos
Copy link
Contributor Author

Good point -- the hash method should not depend on the order.

@ztangent can you please clarify what you mean by "the fixed implementation"?

@georgematheos
Copy link
Contributor Author

Oh, never mind -- I think I see what you mean by that. Got it; I will push a fix along these lines in a minute.

@ztangent
Copy link
Member

Awesome, looks good to me, thanks for fixing this!

@ztangent ztangent merged commit 4fc05a7 into probcomp:master May 21, 2022
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.

None yet

2 participants