Skip to content

Conversation

@oschulz
Copy link
Contributor

@oschulz oschulz commented Jun 14, 2021

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 sunoru merged commit 10ab583 into JuliaRandom:master Jun 15, 2021
@sunoru
Copy link
Member

sunoru commented Jun 15, 2021

Thank you for finding this!

@JuliaRandom JuliaRandom deleted a comment from JuliaRegistrator Jun 15, 2021
@JuliaRandom JuliaRandom deleted a comment from JuliaRegistrator Jun 15, 2021
@oschulz
Copy link
Contributor Author

oschulz commented Jun 15, 2021

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

@oschulz
Copy link
Contributor Author

oschulz commented Jun 15, 2021

@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
Copy link

maleadt commented Jun 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants