-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
Add jump functions (jump
, long_jump
) for Xoshiro
#47743
Add jump functions (jump
, long_jump
) for Xoshiro
#47743
Conversation
Great functionality to have! The names are a bit generic, but they aren't exported, so seems ok to me. Tagged for triage discussion. |
triage approves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! It needs to be rebased, but I checked that the implementation matches the reference.
I would suggest changing the names though: Random.jump(r::MersenneTwister, steps::Integer)
already exists; we haven't documented it, but we might (it's slightly imperfect but probably good enough), and its API is that the steps
argument is the number of skipped Float64
.
But in this PR, the passed steps to jump
specify a multiple of 2^128 UInt64
s which are skipped. Maybe we could implement in the future the machinery to jump ahead by an arbitrary amount (like for MersenneTwister) and/or add more predefined jumps (e.g. this page gives jump polynomials for a bunch of steps), so I feel we should reserve jump
for "arbitrary number of steps, and call the two functions here something like jump_128
and jump_192
instead of jump
and long_jump
.
Let's get this merged soon!
Straightforward implementations given https://xoshiro.di.unimi.it/xoshiro256plusplus.c This is useful functionality which does not interfere with any existing code. Utility aside, it is arguable that the jump functions are necessary to complete the implementation of `xoshiro256++` tooling. In essence, given that the `xoshiro` family supports jump functions, we would be remiss to neglect this capability. For most users, I expect that the existing `TaskLocalRNG` is more than sufficient (hence, why I propose that this not be exported, just like `seed!`). However, if/when one does want to total control, jump functions are a requirement. Use cases arise when one wishes to utilize a single seed (with state advanced sufficiently far as to give non-overlapping subsequences) as the basis for a parallel computation. The alternative is manual seeding, which lacks the flexibility required for testing (imagine a program which requires a variable number of sub-sequences, one for each parallel portion). If further justification is needed, good precedent exists: https://docs.rs/rand_xoshiro/latest/rand_xoshiro/struct.Xoshiro256PlusPlus.html
eb7f6d6
to
cf177a9
Compare
The names have been changed to
Heh, 7-8 months ago I thought that this would just bitrot. |
Test failures look totally unrelated, so merging, thanks @andrewjradcliffe ! |
Straightforward implementations given
https://xoshiro.di.unimi.it/xoshiro256plusplus.c
This is useful functionality which does not interfere with any existing code. Utility aside, it is arguable that the jump functions are necessary to complete the implementation of
xoshiro256++
tooling. In essence, given that thexoshiro
family supports jump functions, we would be remiss to neglect this capability.For most users, I expect that the existing
TaskLocalRNG
is more than sufficient (hence, why I propose that this not be exported, just likeseed!
).However, if/when one does want to total control, jump functions are a requirement. Use cases arise when one wishes to utilize a single seed (with state advanced sufficiently far as to give non-overlapping subsequences) as the basis for a parallel computation. The alternative is manual seeding, which
lacks the flexibility required for testing (imagine a program which requires a variable number of sub-sequences, one for each parallel portion).
If further justification is needed, good precedent exists.