DOC, API: add random.__init__.pxd and document random.* functions #14948
Conversation
| instance's methods are imported into the numpy.random namespace, see | ||
| :ref:`legacy` for the complete list. | ||
|
|
||
| .. _random-quick-start: | ||
|
|
||
| Quick Start | ||
| ----------- | ||
|
|
There was a problem hiding this comment.
Rework the documentation in lines 36-52 to better guide users how to modify existing code to call default_rng
| @@ -121,3 +121,63 @@ Distributions | |||
| ~RandomState.wald | |||
| ~RandomState.weibull | |||
| ~RandomState.zipf | |||
|
|
|||
There was a problem hiding this comment.
Add the legacy functions below so they are linked into the intersphinx index
| .. note:: | ||
| New code should use ``default_rng().random`` (see | ||
| `random-quick-start`) instead. | ||
|
|
There was a problem hiding this comment.
Add this note and the See Also fragment to each of the exposed functions
There was a problem hiding this comment.
Makes sense, short and to the point.
There was a problem hiding this comment.
Per my comment in the random-quick-start, I really don't like the default_rng().whatever() construction. People should not be making a new Generator every line. If it's really important that people have a replacement for the np.random.whatever() calls that fits on one line, make those module-level aliases against a global Generator.
There was a problem hiding this comment.
-
Instantiating a new
Generatoreach time means more reading of/dev/urandom. While we don't pull a lot of data from it, it's still a system call, which is slow, and is going to block other processes trying to read it at the same time. If someone is mechanically replacingnp.random.whatever()withnp.random.default_rng().whatever(), that could be a lot of calls in tight loops. -
I fear that this usage, especially when presented in this context, is going to engrain the idea that
default_rng()returns the defaultGeneratorinstance rather than creating a new one each time since that was the mental model ofnp.random.whatever().
There was a problem hiding this comment.
+1 on these comments. Perhaps just note the new name of the function when using the default_rng rather than suggesting a specific function call?
Perhaps something along the lines of
New code should use the random method of a default_rng() instance (see random-quick-start for more detail).
There was a problem hiding this comment.
is going to engrain the idea that
default_rng()returns the defaultGenerator
Perhaps we got the name wrong, and should call this make_default_rng?
At the very least, this sounds like something that needs to be drawn attention to in the docstring of default_rng.
There was a problem hiding this comment.
There was a problem hiding this comment.
I like the "Use the *** method of a default_rng() instance ..." text. Any other alternatives?
| Original Source | ||
| ~~~~~~~~~~~~~~~ | ||
| Original Source of the Generator and BitGenerators | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| This package was developed independently of NumPy and was integrated in version | ||
| 1.17.0. The original repo is at https://github.com/bashtage/randomgen. |
There was a problem hiding this comment.
Aren't there more bit generators in randomgen? If so, that seems worth mentioning. Otherwise, this subsection is probably best moved to THANKS.txt, it doesn't help the user.
There was a problem hiding this comment.
There are many more there still.
| @@ -121,3 +121,63 @@ Distributions | |||
| ~RandomState.wald | |||
| ~RandomState.weibull | |||
| ~RandomState.zipf | |||
|
|
|||
| Toplevel `numpy.random` functions | |||
There was a problem hiding this comment.
toplevel is kind of an odd word. How about just Functions in numpy.random?
|
|
||
| Toplevel `numpy.random` functions | ||
| ================================= | ||
| Many of the RandomState methods above are exported as top-level `numpy.random` |
There was a problem hiding this comment.
I'd also remove top-level here. And add a mention that using a Generator instance, as can be obtained from default_rng, should be preferred.
|
Looks good, just some minor comments. |
| # Uses the old numpy.random.RandomState | ||
| #Do this | ||
| from numpy.random import default_rng | ||
| val = default_rng().standard_normal() |
There was a problem hiding this comment.
Let's not encourage the default_rng().whatever() construction.
| val = default_rng().standard_normal() | |
| rng = default_rng() | |
| val = rng.standard_normal() |
ab70a9c to
0a5e428
Compare
|
Build failure on PyPy is a unrelated heisenbug |
|
Thanks Matti. If anyone has further changes they would like to see, make a new PR. |
rework parts of gh-14604:
numpy.random.__init__.pxdsofrom numpy.random cimport ...works in cython__init__.pxdRandomStatethat are exposes as functions innumpy.randomto top-level documentation, which should allow them to be referenced via the intersphinx index.