beacon-biosignals / StableHashTraits.jl

Compute hashes over any Julia object simply and reproducibly
MIT License
7 stars 1 forks source link

Module info ignored for ComposedFunctions #54

Closed rasmushenningsson closed 2 months ago

rasmushenningsson commented 7 months ago

It seems like the module information is ignored for ComposedFunctions.

julia> isascii() = true # just to shadow function in Base
isascii (generic function with 1 method)

As expected, these give different hashes:

julia> bytes2hex(stable_hash(Base.isascii; version=2))
"21e86e9b58d17171bef2ea19d0ab5fa719ebb60fef27fe4d094cc74ea4dbf74e"

julia> bytes2hex(stable_hash(isascii; version=2))
"9163d27ca6525cd24900c3b2ed4da3ecc733f4602862d2764101a0c48d9f4672"

But these give the same hashes, even though the objects are distinct:

julia> bytes2hex(stable_hash(!Base.isascii; version=2))
"ea3689b90434e4ae2ba764e585c9a0b432c5d0185da872aba5ee46eba2a72755"

julia> bytes2hex(stable_hash(!isascii; version=2))
"ea3689b90434e4ae2ba764e585c9a0b432c5d0185da872aba5ee46eba2a72755"

It seems like it would be better if they would return different hashes.

haberdashPI commented 7 months ago

Oh... yes. I see. I can investigate this. I think I know what is going on.

However it is possible I will want to merge #55 first, depending on how much of a lift this is to fix, as that PR is a pretty substantial change to the implementation and API.

rasmushenningsson commented 7 months ago

No hurry. It makes sense to merge #55 first.

haberdashPI commented 5 months ago

55 Should address this

haberdashPI commented 3 months ago

Note: in the final design of #55, @ericphanson and I eventually opted to always exclude the module information, as it is often considered an implementation detail of a package what module it belongs to.

The new documentation (merged in #58) aims to clearly communicate that the module information is ignored, and how you can opt in to not ignoring it. @rasmushenningsson, if you have an opportunity to take a look through the section on how Function's are hashed, I'd appreciate any feedback regarding whether you think it is sufficient to close #54.

rasmushenningsson commented 2 months ago

This makes total sense to me. This issue was mainly about consistency. Closes as solved by #58.