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

Added doc to RNG about changing between versions #34670

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Feb 5, 2020

This PR is based on a few people asking about why their tests failed following a minor version change (as far as I can tell, this one). Notably on discourse following 0.6 -> 0.7 (here) and because of the linked PR the tests in SpatialEcology.jl also failed.

Relying on a specific stream of generated numbers using any of the interfaces of Random is imo bad form and should be avoided if possible, so a warning would be useful.

@ViralBShah ViralBShah requested a review from rfourquet February 5, 2020 09:33
@ViralBShah ViralBShah added randomness Random number generation and the Random stdlib docs This change adds or pertains to documentation labels Feb 5, 2020
@mkborregaard
Copy link
Contributor

I was bitten by this recently, my tests suddenly started erroring from julia-1.1 to julia-1.2

@JuliaLang JuliaLang deleted a comment from Seelengrab Feb 5, 2020
@ViralBShah
Copy link
Member

@Seelengrab I just moved your comment to the top of the issue since it greatly helps a reader of the issue.

@@ -26,6 +26,9 @@ unbounded integers, the interval must be specified (e.g. `rand(big.(1:6))`).
Additionally, normal and exponential distributions are implemented for some `AbstractFloat` and
`Complex` types, see [`randn`](@ref) and [`randexp`](@ref) for details.

!!! warn
Because the precise way in which random numbers are generated is considered an implementation detail, bug fixes and speed improvements may change the stream of numbers that are generated after a version change. Relying on a specific seed or generated stream of numbers during unit testing is thus discouraged - consider testing properties of the methods in question instead.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe specify "minor version change" ? (someone else should confirm)

Copy link
Contributor Author

@Seelengrab Seelengrab Feb 5, 2020

Choose a reason for hiding this comment

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

I guess that depends on if RNG is only allowed to change in a minor but not a bug version? Seems weird though, especially since RNG bugs often affect critical assumptions and keeping those broken for longer than necessary seems unwieldy.

Copy link
Member

Choose a reason for hiding this comment

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

Ah good point; improvements changes will happen in minor versions (and be labelled as "minor change"), but a bug fix changing the streams (although rare) might happen in a patch version.

@ViralBShah
Copy link
Member

Merge?

@KristofferC KristofferC merged commit 1f28cc0 into JuliaLang:master Feb 7, 2020
@Seelengrab Seelengrab deleted the rngDoc branch February 7, 2020 07:56
@kimikage
Copy link
Contributor

kimikage commented Aug 2, 2020

Isn't the "warn" a typo of "warning"?

@Seelengrab
Copy link
Contributor Author

It was consistent with other warnings in the docs, e.g. here. IIRC, the keyword for warning in the old documentation system was warn, but that seems to have changed (https://docs.julialang.org/en/v1/stdlib/Markdown/#Admonitions-1). A quick grep through the source shows some other places where the old keyword is, I'll make a new PR to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants