-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
MersenneTwister: clean up constructors #60185
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
base: master
Are you sure you want to change the base?
Conversation
We had two sets of constructors: 1) user-facing ones, which mimic `show` (e.g. `MersenneTwister(seed)`, or `MersenneTwister(1, (0, 1002, 0, 1))`), 2) internal ones: - `MersenneTwister(seed, state, vals, ...)` - `MersenneTwister(seed, state)` Internal ones were not practical to use, so they are replaced by a single `MersenneTwister(undef)` constructor which prepares an uninitialized instance, which can then be initialized by `seed!`, `copy!`, etc. This commit also makes `const` some internal fields, and replaces `Vector` with `Memory` for one of them.
| @assert dsfmt_get_min_array_size() <= MT_CACHE_F | ||
|
|
||
| mutable struct MersenneTwister <: AbstractRNG | ||
| seed::Any |
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.
does a MersenneTwister really need to store it's seed?
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.
Yes looks like this can be deleted.
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.
It's there to recreate the state of an instance from few values, so that what show outputs corresponds to a real constructor. E.g
julia> MersenneTwister("a seed")
MersenneTwister("a seed")But I was thinking to perhaps normalize seeds into an UInt128 value and store that instead of the original seed. The example above would then look like
julia> MersenneTwister("a seed")
MersenneTwister(0xb4802685c420b29be64de36a1f90815f)
julia> MersenneTwister(0xb4802685c420b29be64de36a1f90815f)
MersenneTwister(0xb4802685c420b29be64de36a1f90815f)This would involve an additional round with SeedHasher, but I'm precisely preparing another PR to make that much faster.
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.
what about just having a HashedSeed type?
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.
The user seed is removed in #60204, without an additional round with SeedHasher, by just storing two UInt128 values instead of one. Printing is uglier, but who cares.
|
I don't think exposing an explicit |
That's actually something I've thought to bring as an experimental API in v1.14, but I'm fine postponing its introduction to a future PR if there is push-back here. |
|
What I'm saying is that I don't think having |
|
As you noted, there is already a internal MT constructor which leaves an instance in an uninitialized state (though admitedly it's harder to call because there are many arguments). It's also possible to initialize
Let's leave discussing that for the future PR I mentioned. But in short, there are certain low-level use cases for which it can be convenient to get an unitialized instance, which can be initialized later (similarly to |
Per review comment.
|
I changed |
We had two sets of constructors:
show(e.g.
MersenneTwister(seed), orMersenneTwister(1, (0, 1002, 0, 1))),MersenneTwister(seed, state, vals, ...)MersenneTwister(seed, state)Internal ones were not practical to use, so they are replaced by a single
MersenneTwister(undef)constructor which prepares an uninitialized instance, which can then be initialized byseed!,copy!, etc.This commit also makes
constsome internal fields, and replacesVectorwithMemoryfor one of them.