search

JuliaRandom / Random123.jl

Julia implementation of Random123.
http://juliarandom.github.io/RandomNumbers.jl/stable/man/random123/
Other
18 stars 5 forks source link

Hotfix for incorrect counter increment for R123Generator4x generators #9

Closed oschulz closed 3 years ago

oschulz commented 3 years ago

Random123 v1.4.0 now increments all counter fields (I think #5 can be closed now), but does so incorrectly for R123Generator4x generators: it increments ctr3 in parallel with ctr1:

julia> using Random123

julia> r = Philox4x()
Philox4x{UInt64, 10}(0x85dc666f04ca7117, 0x91f93bdbc1b4ddfd, 0x9aeff0a67d75d113, 0x49c8fa9f501ad38b, 0x1ae41951b41e08b9, 0xa38977fb635aac5c, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0)

julia> (rng.ctr4, rng.ctr3, rng.ctr2, rng.ctr1)
(0x0000000000000000, 0x0000000000000004, 0x0000000000000000, 0x0000000000000005)

julia> Random123.inc_counter!(rng); (rng.ctr4, rng.ctr3, rng.ctr2, rng.ctr1)
(0x0000000000000000, 0x0000000000000005, 0x0000000000000000, 0x0000000000000006)

This break applications (like BAT.jl) that partition the counter space for reproducible parallel processing.

PR contains a fix and increases the package version as well.

@sunoru I'd be super grateful for a quick merge and a v1.4.1 bugfix release, since BAT.jl is currently broken due to this.

sunoru commented 3 years ago

Thank you for finding this!

oschulz commented 3 years ago

Thanks a lot for the quick merge and release, @sunoru !

oschulz commented 3 years ago

@maleadt in case you have GPU-related plans here, should I rewrite inc_counter! in a branch-free fashion? Shouldn't be too hard.

maleadt commented 3 years ago

The GPU implementation is a bit special, I currently set ctr2 to the thread ID, https://github.com/JuliaGPU/CUDA.jl/blob/537be6c430decca43d3b4c3fafc09d445f29040c/src/device/random.jl#L75-L80, and only increment ctr1, https://github.com/JuliaGPU/CUDA.jl/blob/537be6c430decca43d3b4c3fafc09d445f29040c/src/device/random.jl#L138; so I don't use inc_counter!.