-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
API: restructure and document numpy.random C-API #14604
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
Conversation
Still a WIP. Need to decide how much of the Edit: pxd -> c-extension module |
xref gh-5312 for moving the aligned allocations into numpy itself |
Sorry this is a bit long for a PR comment. Should it be an issue? Line 75 says "An example ... is in the examples folder" which is part of the sources under It contains examples of using the BitGenerator interfaces I am not a numba expert. One of the examples is very close to the one in the TBD:
Are we answering the user stories in these examples? One of the mailing list comments linked to the scipy.special documentation, should we try to create a page like that? User stories - we should try to relate to these (from the mailing list thread)Someone asked how to use random in a ufunc. "An ideal API would allow projects like https://github.com/deepmind/torch-randomkit/tree/master/randomkit or numba to consume the code in NumPy without vendoring it." "There are c++ applications which use boost::random, would be nice to be able to swap it for numpy.random." "Using the existing distributions from Cython was a requested feature and an explicit goal, yes. There are users waiting for this." (mattip: but isn't this supported via "Numba would definitely appreciate C functions to access the random distribution implementations, and have a side-project (numba-scipy) that is making the Cython wrapped functions in SciPy visible to Numba" (mattip: I think we do this after gh-14608 from _generator.so via these Maybe someone who wants to write a new BitGenerator, i.e., the numpy/bitgenerator repo without needing to have the numpy code as a git submodule |
def normals_zig(Py_ssize_t n): | ||
|
||
random_values[i] = random_gauss_zig(rng) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the _zig
is no longer in the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole block should be included via a .. literalinclude
directive from the appropriate example file
Might indeed make sense to merge this quickly (there's only some doc fixes) here that improve the current state of
Good point. I'd suggest moving the examples into the documentation, shipping them in the tarball and including them in the rendered docs as well.
I think so. It's no different than any other public function in the Python and C API: every function and object should be listed.
Nice collection. Yes, those would make good examples/subsections in the user guide section on this topic. |
@@ -19,7 +19,7 @@ The included BitGenerators are: | |||
and can be advanced by an arbitrary amount. See the documentation for | |||
:meth:`~.PCG64.advance`. PCG-64 has a period of :math:`2^{128}`. See the `PCG | |||
author's page`_ for more details about this class of PRNG. | |||
* MT19937 - The standard Python BitGenerator. Adds a `~mt19937.MT19937.jumped` | |||
* MT19937 - The standard Python BitGenerator. Adds a `MT19937.jumped` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -32,7 +32,7 @@ instance's methods are imported into the numpy.random namespace, see | |||
Quick Start | |||
----------- | |||
|
|||
By default, `~Generator` uses bits provided by `~pcg64.PCG64` which | |||
By default, `~Generator` uses bits provided by `PCG64` which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment elsewhere about bad refs from #14608
The cython code does not compile, it needs |
91819d8
to
766615b
Compare
Something is fishy with macos Azure runs. It is printing:
I think this is the result of Anyone with MacOS care to try to duplicate it? @tylerjereddy any ideas? |
In 37bbe58 I added |
I removed the WIP from the title. The example works, we have the new c-api documentation (link in the comment above) and examples both in documentation and in tests. Reviews would be welcome. More examples and documentation will be generated in gh-14778 |
|
||
.. currentmodule:: numpy.random | ||
|
||
Typed versions of many of the `Generator` and `BitGenerator` can be accessed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing. What does it mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing fragment. It should read "Typed versions of many of the methods and functions in Generator
and BitGenerator
can be accessed ...
Fixing
Typed versions of many of the `Generator` and `BitGenerator` can be accessed | ||
directly from Cython: the complete list is given below. | ||
|
||
The ``_bit_generator`` module is usable via:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "is available".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, and again below
Thanks Matti. I'm not convinced that the examples are in the right place but they can probably be moved later without problems. |
The examples might better be included somewhere in the |
I put the examples there so they could be tested. Pieces of them are |
That sounds like a good idea actually. Please move them to |
Finish the C-API side of the new numpy.random API