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

test/random.jl: segregate deterministic tests at the end #8339

Closed
wants to merge 1 commit into from

Conversation

rfourquet
Copy link
Member

See #8313 for context and in particular #8313 (comment) for the rationale of this change.
In short, the first test in "test/random.jl", which needed to seed the global RNG with a deterministic value (srand(0)), was leaking determinism on all the tests coming after it.

@ivarne
Copy link
Sponsor Member

ivarne commented Sep 15, 2014

How would you get the RANDOM_SEED.txt file from a failed travis run? We also need a way to rerun the tests with the failing seed.

One solution to the first issue might be to create a @test_random macro that prints Base.Random.RANDOM_SEED on test failure and otherwise behaves like @test.

@StefanKarpinski
Copy link
Sponsor Member

Printing the RNG state right before the failed test would be even better. That way you can reproduce the failure without running any other tests.

@ivarne
Copy link
Sponsor Member

ivarne commented Sep 15, 2014

That would require a method to get and set the internal RNG state. Do we even want that to be part of the API? Tests also often depends on variables declared at an earlier point of the test file, so you would have to run the other tests as well.

@StefanKarpinski
Copy link
Sponsor Member

Well, I would want it. Not sure how many RNGs allow it. Setting up local variables is possible without running everything before, especially now that we print better info in failed tests.

@andreasnoack
Copy link
Member

The state is stored as a Vector{Int32} in a DSFMT_state type and therefore it is fairly easy to get and set. However, it is important that you don't use a setstate! method as a seeding mechanism because the Mersenne-Twister can work poorly for some states. Therefore the library has specialized seeding functions that chooses a good state from a Uint32 seed.

I agree, that it would be useful to have the state printed for failing tests.

@rfourquet
Copy link
Member Author

I of course didn't think of Travis...
The random.jl tests are not that long to run, so it's not a too big problem if they have to be rerun when there is a failure, until a @test_random macro is implemented (I mean that the person trying to debug can then e.g. manually insert prints statements to get the RNG state just before the failed test).
Would you suggest that I remove the writing to RANDOM_SEED.txt from this commit?


using Base.Random.RANDOM_SEED

srand()
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like?

srand() #Reseed the RNG with entropy from the OS
seed = rand(Uint)
#seed = 
println("Running random tests with seed\nseed = 0x",hex(seed))
srand(seed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you would like seeding with rand(Uint)... is it so to be able to print only an Uint instead of RANDOM_SEED which is an array of four Uint32 ?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was for convenience and shorter printing. If you think there are any benefit in using RANDOM_SEED directly, please do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would then prefer to print RANDOM_SEED as an Uint128. So do you suggest I update this PR to print to the screen instead of to the file?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for Travis and bug reports, it seems better to print to STDOUT. Uint128 seems like a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ViralBShah
Copy link
Member

What's the final word here?

@rfourquet
Copy link
Member Author

I think that there the problem is still there (most tests don't "require" the srand(0) at the beginning of the file, and would then be more useful if run with different seeds each time), but this PR may not be the best solution now that the rand API has been extended. I need to think more about it, but I would suggest that the global RNG is not re-seeded (except to test srand(seed), but then the original seed would be set back after the test), and that tests using srand use also an explicit MersenneTwister instance. I will make an updated PR soon.

@rfourquet
Copy link
Member Author

Superseeded by #16924 and #16940 (pun intended).

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.

5 participants