Skip to content

Commit

Permalink
fix bug with rand(::MersenneTwister, ::UInt128) (#37808)
Browse files Browse the repository at this point in the history
Generation of UInt64 and UInt128 share the same cache, but the routine
handling generation of UInt128 was not fully aknowledging the sharing.
This leads to situations like:

```
julia> m = MersenneTwister(0); rand(m, UInt64); rand(m, UInt128)
0x79ed9db9ec79a6a019c5f638a776ab3c

julia> rand(m, UInt64)
0x19c5f638a776ab3c
```
These values aren't independent enough!
  • Loading branch information
rfourquet authored Oct 3, 2020
1 parent 1b3ddd4 commit 9392bbe
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 4 deletions.
7 changes: 3 additions & 4 deletions stdlib/Random/src/RNGs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -232,12 +232,11 @@ function mt_pop!(r::MersenneTwister, ::Type{T}) where T<:BitInteger
(x128 >> (i128 * (sizeof(T) << 3))) % T
end

# not necessary, but very slightly more efficient
function mt_pop!(r::MersenneTwister, ::Type{T}) where {T<:Union{Int128,UInt128}}
reserve1(r, T)
@inbounds res = r.ints[r.idxI >> 4]
r.idxI -= 16
res % T
idx = r.idxI >> 4
r.idxI = idx << 4 - 16
@inbounds r.ints[idx] % T
end


Expand Down
32 changes: 32 additions & 0 deletions stdlib/Random/test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -801,3 +801,35 @@ end
@testset "RNGs broadcast as scalars: T" for T in (MersenneTwister, RandomDevice)
@test length.(rand.(T(), 1:3)) == 1:3
end

@testset "generated scalar integers do not overlap" begin
m = MersenneTwister()
xs = reinterpret(UInt64, m.ints)
x = rand(m, UInt128) # m.idxI % 16 == 0
@test x % UInt64 == xs[end-1]
x = rand(m, UInt64)
@test x == xs[end-2]
x = rand(m, UInt64)
@test x == xs[end-3]
x = rand(m, UInt64)
@test x == xs[end-4]
x = rand(m, UInt128) # m.idxI % 16 == 8
@test (x >> 64) % UInt64 == xs[end-6]
@test x % UInt64 == xs[end-7]
x = rand(m, UInt64)
@test x == xs[end-8] # should not be == xs[end-7]

s = Set{UInt64}()
n = 0
for _=1:2000
x = rand(m, rand((UInt64, UInt128, Int64, Int128)))
if sizeof(x) == 8
push!(s, x % UInt64)
n += 1
else
push!(s, x % UInt64, (x >> 64) % UInt64)
n += 2
end
end
@test length(s) == n
end

4 comments on commit 9392bbe

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Please sign in to comment.