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.

Sign up for GitHub

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

make hash(::Xoshiro) compatible with == #51376

Merged
merged 1 commit into from
Sep 22, 2023
Merged

make hash(::Xoshiro) compatible with == #51376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions stdlib/Random/src/Xoshiro.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ 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)
# use a magic (random) number to scramble `h` so that `hash(x)` is distinct from `hash(getstate(x))`
hash(x::Union{TaskLocalRNG, Xoshiro}, h::UInt) = hash(getstate(x), h + 0x49a62c2dda6fa9be % UInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the magic number for? If it's required, please document why it's there.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

one reason to chose a constant is that you want to make sure the constant doesn't have small prime factors due to superstition.

Copy link
Contributor

@Seelengrab Seelengrab Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"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.


function seed!(rng::Union{TaskLocalRNG, Xoshiro})
# as we get good randomness from RandomDevice, we can skip hashing
Expand Down
51 changes: 34 additions & 17 deletions stdlib/Random/test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,24 +594,41 @@ guardseed() do
Random.seed!(typemax(UInt128))
end

# copy, == and hash
let seed = rand(UInt32, 10)
r = MersenneTwister(seed)
@test r == MersenneTwister(seed) # r.vals should be all zeros
@test hash(r) == hash(MersenneTwister(seed))
s = copy(r)
@test s == r && s !== r
@test hash(s) == hash(r)
skip, len = rand(0:2000, 2)
for j=1:skip
rand(r)
rand(s)
@testset "copy, == and hash" begin
for RNG = (MersenneTwister, Xoshiro)
seed = rand(UInt32, 10)
r = RNG(seed)
t = RNG(seed)
@test r == t
@test hash(r) == hash(t)
s = copy(r)
@test s == r == t && s !== r
@test hash(s) == hash(r)
skip, len = rand(0:2000, 2)
for j=1:skip
rand(r)
@test r != s
@test hash(r) != hash(s)
rand(s)
end
@test rand(r, len) == rand(s, len)
@test s == r
@test hash(s) == hash(r)
h = rand(UInt)
@test hash(s, h) == hash(r, h)
if RNG == Xoshiro
t = copy(TaskLocalRNG())
@test hash(t) == hash(TaskLocalRNG())
@test hash(t, h) == hash(TaskLocalRNG(), h)
x = rand()
@test hash(t) != hash(TaskLocalRNG())
@test rand(t) == x
@test hash(t) == hash(TaskLocalRNG())
copy!(TaskLocalRNG(), r)
@test hash(TaskLocalRNG()) == hash(r)
@test TaskLocalRNG() == r
end
end
@test rand(r, len) == rand(s, len)
@test s == r
@test hash(s) == hash(r)
h = rand(UInt)
@test hash(s, h) == hash(r, h)
end

# MersenneTwister initialization with invalid values
Expand Down