to replace random.seed(seed=None) using dedicated Generator instance(… #25765
Conversation
…according to numpy reference)
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
| @@ -11,12 +11,14 @@ | |||
| plt.style.use('_mpl-gallery') | |||
|
|
|||
| # make the data | |||
| rng = np.random.default_rng(seed=None) | |||
There was a problem hiding this comment.
| rng = np.random.default_rng(seed=None) | |
| rng = np.random.default_rng(seed=19680801) |
We use 19680801 as the seed anyplace we have to put in a seed.
There was a problem hiding this comment.
This is not true for the "Plot types" examples. They all use 3, likely because they were taken from the cheat sheets. I'm in (weak) favour of keeping that (a) for consistency and (b) because 19680801 is more noisy.
There was a problem hiding this comment.
@qqwqqw689 This needs a seed. I'd use "3" in this case as well.
I'm not clear why you have only changed one example.
|
Thank you for your work on this, however I am a bit torn. The point of this example is to show how to use scatter not how to use numpy rng machinery. Would it be better to change the data all together to avoid it? Just how strongly do the numpy devs want us to stop using the old way? |
|
The following is the original sentence. This is a convenience, legacy function(i.e. numpy.random.seed) that exists to support older code that uses the singleton RandomState. Best practice is to use a dedicated Generator instance rather than the random variate generation methods exposed directly in the random module. |
|
Similar discussion at #19706, although back then the newness of the generators was also a concern. |
https://numpy.org/doc/stable/reference/random/legacy.html#functions-in-numpy-random states
As of today I'm +/-0 on switching. While I like the old functional approach a bit better (feels simpler than creating rng state explicitly), the recommendation is clear and we should switch ultimately. I would not have actively pushed for the switch yet, but the new random generators are old enough now, so that there are no practical concerns. |
There are quite a lot of them... |
|
I'm not in favour of a bunch of churn to "fix" these. OTOH, new issues should probably use the new generators, and folks could clean these up if doing other work to the example. |
|
I am now leaning towards switching all of our examples to use the new style RNGs. There are good technical reasons to switch and as we would like our down-stream projects to switch to using the explicit API in their docs, we should reciprocate when our up-streams make a similar request. |
While looking for something else, I saw that ruff can do this for us 👀 Edit: actually it is just a check rather than an autofix… |
…according to numpy reference)
PR Summary
PR Checklist
Linked Issue
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst