Skip to content
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

Make srand work similarly for global RNG and MersenneTwister instance #8331

Closed
wants to merge 2 commits into from

Conversation

rfourquet
Copy link
Member

The docs pretended it did, but were lying.
Also, the first commit simplifies (see commit message) MersenneTwister in order to homogeneize how its constructor and its srand method work (the constructor now calls srand).

I would like to add another change (which is more breaking), but need feed-back: MersenneTwister() returns always a "similiar" instance by calling MersenneTwister(0) (I find this surprising), whereas srand(r::MersenneTwister) seeds r with the help of OS' provided entropy. This is not consistent and I'd prefer the second behavior for both methods (i.e. MersenneTwister() = (m = MersenneTwister(0); srand(m); m) or equivalent).
In fact I would like MersenneTwister having a unique constructor with unconstrained parameter which calls srand (boils down to adding string to the possible parameter types).

@rfourquet
Copy link
Member Author

I had also one question: is there a particular reason why using the global RNG controlled by SFMT is preferred over handling a global MersenneTwister insance on the Julia side?

@andreasnoack
Copy link
Member

For the last question, I think it is faster to use the global RNG controlled by DSFMT.

Wouldn't it make sense to feed the seed to the MersenneTwister constructor instead of having an srand method for RNGs?

@rfourquet
Copy link
Member Author

@andreasnoack Thanks for your answer, I'll probably test the speed difference someday.
I think your proposition can make sense, but then it would not be possible to reseed an existing instance, which I don't find too limiting (however I just checked a 3kB state array per instance, so why not reusing the same instance when one wants to re-seed?).

@ViralBShah
Copy link
Member

We should definitely test the performance.

@rfourquet
Copy link
Member Author

I forgot one good reason to keep srand in addition to constructor: when other RNGs are implemented, srand(r,seed) is better for generic code than r=MersenneTwister(seed).

@andreasnoack
Copy link
Member

@rfourquet That might be right, but different RGNs might have different seeding types, but let's postpone that discussion to the time when we have more RNGs.

MersenneTwister's constructor making an instance with an Uint32 seed
were removed in commit 4fb4622. But a subsequent call to
srand(r::MersenneTwister, seed) would only accept an Uint32 as a seed,
and would always make r.seed an Uint32; this was not consistent.
The srand methods working on a MersenneTwister instance did not catch
up with those working on the global RNG. They are now made similar
thanks to the make_seed function, which implements the different
logics.
Also, the manual is more precise on the types of valid seeds.
@ivarne
Copy link
Member

ivarne commented Sep 15, 2014

In future API discussions, we must also consider thread safety. Can srand() with a known seed for reproducibility really work in a multi threaded environment?

@rfourquet
Copy link
Member Author

I think srand(seed) in a multithreaded environment should be very rarely used, and that srand has to handle a lock itself, unless the "global" rng becomes thread-local. Multi-threaded programs should probably use non-global rng instances.

@rfourquet
Copy link
Member Author

Closing since this PR is superseded by #8380 which extends this work to the rand[!] functions.

@rfourquet rfourquet closed this Sep 17, 2014
@rfourquet rfourquet deleted the random-srand branch September 17, 2014 06:57
@ViralBShah ViralBShah added the randomness Random number generation and the Random stdlib label Nov 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants