make hash(::Xoshiro) compatible with ==
#51376
Conversation
rfourquet
added
randomness
| @@ -122,6 +122,7 @@ rng_native_52(::TaskLocalRNG) = UInt64 | |||
| copy(rng::Union{TaskLocalRNG, Xoshiro}) = Xoshiro(getstate(rng)...) | |||
| copy!(dst::Union{TaskLocalRNG, Xoshiro}, src::Union{TaskLocalRNG, Xoshiro}) = setstate!(dst, getstate(src)) | |||
| ==(x::Union{TaskLocalRNG, Xoshiro}, y::Union{TaskLocalRNG, Xoshiro}) = getstate(x) == getstate(y) | |||
| hash(x::Union{TaskLocalRNG, Xoshiro}, h::UInt) = hash(getstate(x), h + 0x49a62c2dda6fa9be % UInt) | |||
There was a problem hiding this comment.
What's the magic number for? If it's required, please document why it's there.
There was a problem hiding this comment.
It's essentially so that hash(xoshiro) != hash(getstate(xoshiro)), i.e. the hash depends not only on the raw contained data, but also on the "meaning" of the data, i.e. the type. At least I believe that's the reason why we do this kind of things for a bunch of base types, look at Base.hasha_seed (abstractdict.jl), hashs_seed (set.jl), hash_abstractarray_seed, hashre_seed. But it's not required per se.
There was a problem hiding this comment.
None of those have a comment for why or how those constants are chosen 😔 At the very least, those constructions actively choose a different value when on non-64-bit Julia; it's not just % UInt:
julia> const hashre_seed = UInt === UInt64 ? 0x67e195eb8555e72d : 0xe32373e4
0x67e195eb8555e72d
julia> hashre_seed % UInt32
0x8555e72d
(from the regex hashing.)
Reasoning is good, but should be documented with a comment to reduce the number of undocumented magic numbers in Base.
There was a problem hiding this comment.
Yes I don't understand why these choose a totally different value on 32 vs 64 bits platform, as opposed to what I did here (magic % UInt). OK I can leave a terse comment here.
There was a problem hiding this comment.
one reason to chose a constant is that you want to make sure the constant doesn't have small prime factors due to superstition.
There was a problem hiding this comment.
"a" constant doesn't provide that though - nothing up my sleeve numbers are certainly not randomly chosen, and if that's the intent, these numbers fail that principle (and should be even better documented).
E.g. The 32 bit number from hashre_seed above is even, so has an incredibly small prime factor in 2. Not that it really matters for this.
rfourquet
force-pushed
the
rf/xoshiro-hash
branch
from
03be0ee
to
d662500
Compare
Backported PRs: - [x] #50932 <!-- types: fix hash values of Vararg --> - [x] #50975 <!-- Use rr-safe `nopl; rdtsc` sequence --> - [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16, Float32})` --> - [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple --> - [x] #51332 <!-- Add s4 field to Xoshiro --> - [x] #51397 <!-- call Pkg precompile hook in latest world --> - [x] #51405 <!-- Remove fallback that assigns a module to inlined frames. --> - [x] #51491 <!-- Throw clearer ArgumentError for strip with two string args --> - [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe download error) --> - [x] #51541 <!-- Fix string index error in tab completion code --> - [x] #51530 <!-- Don't mark nonlocal symbols as hidden --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51512 <!-- avoid limiting Type{Any} to Type --> - [x] #51595 <!-- reset `maxprobe` on `empty!` --> - [x] #51582 <!-- Aggressive constprop in LinearAlgebra.wrap --> - [x] #51592 <!-- correctly track element pointer in heap snapshot --> - [x] #51326 <!-- complete false & true more generally as vals --> - [x] #51376 <!-- make `hash(::Xoshiro)` compatible with `==` --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51845 - [x] #51840 - [x] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [x] #51863 <!-- LLVM 15.0.7-9 --> Contains multiple commits, manual intervention needed: - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> Non-merged PRs with backport label: - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia --> - [ ] #51414 <!-- improvements on GC scheduler shutdown --> - [ ] #51366 <!-- Handle infix operators in REPL completion --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions in Base --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
KristofferC
removed
the
backport 1.10
No description provided.