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

deprecate srand(rng, filename::AbstractString, n=4) #21359

Merged
merged 1 commit into from
May 12, 2017

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Apr 12, 2017

This method

  • is never used in practice, I guess, which probably makes its maintenance cost not worth it.

  • was introduced in b13d037 (cc @ViralBShah), mainly (apparently) to be able to seed from "/dev/urandom"; we now have a direct abstraction with RandomDevice (e.g. srand(rng, rand(RandomDevice(), UInt32, 4)), when srand(rng) is not enough).

  • prevents the introduction of a more natural srand(rng, seed::String). srand should only accept a direct seed as second argument (for less confusion); here filename is an indirect seed, and hence requires an extra explanation in the docstring. Moreover, it would be easy to use srand using a file if the above method existed (e.g. srand(rng, read("/dev/urandom", UInt32, 4)) but not the other way around).

  • is not fully satisfying to use: e.g. if n is too big for the file (a finite file, i.e. not "/dev/urandom"), an error is issued, and there is no way to tell srand to read exactly the whole file (I agree that this argument is not strong, we could simply improve this method instead of deprecating it).

If this method is nonetheless deemed to be useful enough that we want to keep its functionality, I propose to make the filename a keyword argument, allowing the introduction later of srand(rng, seed::String).

@rfourquet rfourquet added the randomness Random number generation and the Random stdlib label Apr 12, 2017
@rfourquet
Copy link
Member Author

The deprecation mechanism does not work as I expect:

  • the deprecation message is shown each time I call srand("/dev/urandom", 4), instead of only once;
  • if I call MersenneTwister() (also deprecated) and then MersenneTwister("/dev/urandom"), no message is shown for the latter call (and vice-versa).

Help requested!

@ViralBShah
Copy link
Member

I am ok with deprecating this.

@tkelman
Copy link
Contributor

tkelman commented Apr 12, 2017

I don't think we should do this right now though, we're just about ready for a release candidate. Should wait until we branch.

@JeffBezanson JeffBezanson added the deprecation This change introduces or involves a deprecation label Apr 27, 2017
@rfourquet
Copy link
Member Author

I rebased because of conflicts, and introduced a new section for v0.7.0 in "NEWS.md", please tell me if this is not the way.

@@ -1310,6 +1310,11 @@ end
# PR #16984
@deprecate MersenneTwister() MersenneTwister(0)

# PR #21359
Copy link
Contributor

Choose a reason for hiding this comment

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

these need to be in a new 0.7 section

Copy link
Member Author

Choose a reason for hiding this comment

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

OK done.

@rfourquet rfourquet force-pushed the rf/srand-filename branch 2 times, most recently from 93e8eb1 to dc2bab7 Compare May 10, 2017 10:22
@rfourquet
Copy link
Member Author

I would like to merge this in 1 or 2 days, if no objection comes.

`srand` should accept a seed as second argument; here `filename`
is only an indirect seed, preventing the use a string object as
a direct seed.
@rfourquet rfourquet merged commit ea524c6 into master May 12, 2017
@rfourquet rfourquet deleted the rf/srand-filename branch May 12, 2017 17:13
rfourquet added a commit that referenced this pull request Sep 30, 2023
We used to be able to seed RNGs with a string, but that string was interpreted
as the filename containing the seed. This was deprecated in #21359, in order
to allow later using a string seed directly, which this patch does.
rfourquet added a commit that referenced this pull request Sep 30, 2023
We used to be able to seed RNGs with a string, but that string was interpreted
as the filename containing the actual seed. This was deprecated in #21359, in
order to later allow using a string seed directly, which this patch does.
rfourquet added a commit that referenced this pull request Oct 1, 2023
We used to be able to seed RNGs with a string, but that string was interpreted
as the filename containing the actual seed. This was deprecated in #21359, in
order to later allow using a string seed directly, which this patch does.
rfourquet added a commit that referenced this pull request Oct 7, 2023
We used to be able to seed RNGs with a string, but that string was interpreted
as the filename containing the actual seed. This was deprecated in #21359, in
order to later allow using a string seed directly, which this patch does.
rfourquet added a commit that referenced this pull request Oct 10, 2023
We used to be able to seed RNGs with a string, but that string was
interpreted as the filename containing the actual seed. This was
deprecated in #21359, in order to later allow using a string seed
directly, which this patch does.

---------

Co-authored-by: Nathan Zimmerberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants