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

possible test failure in upcoming Julia version 1.5 #306

Closed
rfourquet opened this issue Apr 24, 2020 · 10 comments · Fixed by #314
Closed

possible test failure in upcoming Julia version 1.5 #306

rfourquet opened this issue Apr 24, 2020 · 10 comments · Fixed by #314

Comments

@rfourquet
Copy link

A PkgEval run for a PR which changes the generated numbers for randn! indicates that the tests of this package might fail in Julia 1.5 (and on Julia current master). Apologies if this is a false positive.
cf. https://github.com/JuliaCI/NanosoldierReports/blob/7de24e455342298cbef56826b5827f0d7640d2c1/pkgeval/by_hash/b89e35c_vs_098ef24/logs/MixedModels/1.5.0-DEV-71a4a114c2.log

@palday
Copy link
Member

palday commented Apr 24, 2020

Thanks for the heads-up. We could probably relax the tolerances for that test a bit, if we want compatibility with Julia 1.4 and 1.5.

I'm a little bit confused about the change in the behavior though. I understand the change in allocation, but given a particular seed, the whole point of PRNG is that you will always get the same sequence of values. If you don't, then you've changed the PRNG and it's no longer the MersenneTwister, but rather the MersenneTwisterrrrr or something.

@rfourquet
Copy link
Author

Cf. https://docs.julialang.org/en/v1.4-dev/stdlib/Random/#Reproducibility-1, there are also discussions on Github. Sorry for the inconvenience!

@palday
Copy link
Member

palday commented Apr 24, 2020

That says why it's allowed, but again, for a given PRNG and seed, the behaviour should be deterministic. I don't think the behavior of an well-defined algorithm like the MersenneTwister should be implementation dependent (assuming all implementations are using IEEE 754 floating point) -- and I know, I know that the algorithm itself doesn't specify how things like associativity should be handled in floating point. But that's a different issue altogether, I guess. I also get pointlessly grumpy when an optimizer gives different answers on different operating systems or processor architectures and this is the same type of 'problem'.

Again, thanks for the heads up! This would have been a confusing test failure otherwise. 😄

@rfourquet
Copy link
Author

for a given PRNG and seed, the behaviour should be deterministic

The Mersenne Twister algorithm is quite standardised, but only for uniform generation. How this is transformed into a normal distribution is dependent on other algorithms. But anyway, the choice have been made now that we favor flexibility (performance improvement and bug fixes) over absolute reproducibility. We could imagine having a dedicated RNG which we try as much as we can to not modify, for better reproducibility.

Again, thanks for the heads up! This would have been a confusing test failure otherwise. smile

You're welcome :)

@rfourquet
Copy link
Author

We could imagine having a dedicated RNG which we try as much as we can to not modify, for better reproducibility.

That has happened, if you would like to try it: https://github.com/rfourquet/StableRNGs.jl

@palday
Copy link
Member

palday commented May 4, 2020

@rfourquet Thanks for the tip.

@dmbates How do you feel about having StableRNGs as an extra testing dependency? Or should we just increase the tolerance in tests based on RNGs? I'm see pros and cons either way.

@dmbates
Copy link
Collaborator

dmbates commented May 4, 2020

If we don't use StablesNRGs.jl can we expect the v1.5 results to be reproducible going forward? In other words, can we condition the tests on VERSION < v"1.5" to allow for both the old and the new results?

@palday
Copy link
Member

palday commented May 4, 2020

@dmbates My understanding of the policy is that no such guarantees exist even between minor versions. Basically for anything but the raw output of a given PRNG like MersenneTwister, the only guarantees for things like randn are the distributional ones (i.e the same you would get by changing a seed within versions).

@rfourquet
Copy link
Author

Basically for anything but the raw output of a given PRNG like MersenneTwister

Even the "raw output" can be changed, if "raw" means things like rand(Float64). This seems not very likely at this stage, but could happen. Also, another RNG than MersenneTwister migth become the new default, there is an open PR for that (but I don't know whether this would affect MixedModels).

@palday
Copy link
Member

palday commented May 4, 2020

@rfourquet I think we explicitly invoke MersenneTwister for most things, so changing the default at that level wouldn't change much for us.

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

Successfully merging a pull request may close this issue.

3 participants