API: rearrange the cython files in numpy.random by mattip · Pull Request #14608 · numpy/numpy

mattip · 2019-09-27T14:23:35Z

This is the code part of #14604 to cleanup the api and allow numpy.random to be used from cython.

Principles:

since the pxd files define the cython API, any public except mtrand *.so should ship a *.pxd and any *.pxd should have a *.so. Any "private" *.so should begin with _. Edit: qualify to state "public except mtrand", which cannot be made private as per NEP 19. We only have private c-extension modules now, so there are no pxd files to ship.
move h files needed by the pxd files into numpy/random/include
~~ship the pxd files and headers~~ Edit: there are no pxd or headers to ship

Cleanup

common -> _common
bound_integers -> _bound_integers ?
don't import *
remove libc imports from _common, let each pxd import on its own
move bitgen_t from common to bit_generator.pxd
fold distributions.pxd into generator.pxd, legacy_distributions.pxd into mtrans.pxd.

Renaming and removing unused functions in distributions.h and legacy_distributions.h

random_gauss_zig -> random_standard_normal
all the others should loose the zig suffix
edit: except random_standard_exponential_zig
make the float versions consistent with _f suffix
random_double -> ~~random_random for consistency with random.random~~ random_standard_uniform
functions that are not used should be removed
random_geometric_* do not need to be exposed
cleanup gauss names

After this cleanup we can create some cython user stories and check that the API is sufficient.

mattip · 2019-09-27T14:24:09Z

The first changeset moves things around without any function renaming yet.

mattip · 2019-09-27T14:25:53Z

numpy/random/setup.py

Still need to create these pxd files so users can do from pcg64 cimport PCG64 to use the BitGenerator directly from cython

Why not keep this in the main namespace? PCG64 is in numpy.random so I'd much prefer it not be in separate pxd files here

main namespace

This mimics the way we import the BitGenerators now.
Do you mean we should refactor the module so that __init__.py imports differently?

That's already done remember? gh-14360

I am still seeing the BitGenerators in both the c-extension module and as imported by __init__.py:

>>> np.random.mt19937.MT19937 <class 'numpy.random.mt19937.MT19937'> >>> np.random.MT19937 <class 'numpy.random.mt19937.MT19937'> >>>

I am not sure we can flatten the namespace without unifying all the c-extension modules into one

Hmm, missing some more underscores.

I am not sure we can flatten the namespace without unifying all the c-extension modules into one

Of course we can, it's just a matter of adding underscores to the .pyx files.

made private in 9bdf1749e. mtrand must remain public as per NEP 19

... the global RandomState instance MUST be accessible in this initial release by the name numpy.random.mtrand._rand ...

rgommers · 2019-09-28T08:14:55Z

Those principles all sound good, thanks @mattip.

rgommers · 2019-09-28T08:17:07Z

legacy_distributions.pxd into mtrans.pxd.

There was no reply yet to your mailing list question on whether these really need to be exposed. Your preference on the mailing list was "no" I believe. I'd like to see a good rationale too before we expose the legacy generator.

mattip · 2019-09-28T21:45:33Z

the next commit moves common, bounded_integers to _common, _bounded_integers and removes all * imports

mattip · 2019-10-07T09:09:15Z

I am going back and forth about shipping pxd files for the BitGenerators Philox, PCG64, MT19937, and SFC64. I am not sure I see the use case for cimport Philox. I think I would prefer to modify the goal to "any public so should have a pxd", which would be bit_generator.pxd, generator.pxd and mtrand.pxd. Those three already exist.

rgommers · 2019-10-07T09:29:13Z

I think I would prefer to modify the goal to "any public

I think that would be better indeed.

so should have a pxd", which would be bit_generator.pxd, generator.pxd and mtrand.pxd. Those three already exist.

I still don't quite understand why generator and bit_generator should be public. From __init__.py:

from .generator import Generator, default_rng
from .bit_generator import SeedSequence

Is there anything else in those two submodules that is public but not imported into the numpy.random namespace?

mattip · 2019-10-08T10:38:43Z

it seems not. I would like to get more input though, @bashtage, @rkern? Can we prepend _ to bit_generator and _generator, leaving mtrand as the only "public" module? Would we then still ship _bit_generator.pxd, _generator.pxd?

…leanup

mattip · 2019-10-11T12:30:05Z

I think this is ready for review now. Note there are no public c-extension modules, all of them (except mtrand, which is private but cannot be renamed as per NEP 19) are private. Once this is merged, we can revisit #14604 to try to develop the correct C-API.

rgommers · 2019-10-11T12:48:45Z

Awesome, thanks Matti.

Once this is merged, we can revisit #14604 to try to develop the correct C-API.

sounds like a good plan

rgommers · 2019-10-12T14:35:54Z

doc/source/reference/random/bit_generators/index.rst

-    bit_generator.ISeedSequence
-    bit_generator.ISpawnableSeedSequence
-    bit_generator.SeedlessSeedSequence
+    _bit_generator.ISeedSequence


This looks wrong. You should never have something starting with an underscore in a toctree listing (or anywhere in the docs really).

So when I asked if there was anything in bit_generator besides SeedSequence that needs to be public, I guess the right answer was not "no" but "ISeedSequence, ISpawnableSeedSequence and SeedlessSeedSequence" (these are needed for writing other bitgenerators outside of NumPy, right?)?

No, they are needed to implement new SeedSequence interfaces as kind of an abstract base class. BitGenerators should use bit_generator.SeedSequence. I think we should remove these then, and maybe document them in the (as yet nonexistant) random C-API documentation, not in the random python reference documentation.

If they're not supposed to be used from Python at all, but only from Cython/C for new BitGenerator authors, then that sounds like the right course of action.

What should we do with text like this "One may also pass in an implementor of the ISeedSequence interface like SeedSequence"? How about simplifying it to not mention ISeedSequence ?

adopted the "don't mention" approach for now.

that sounds good to me

doc/source/reference/random/new-or-different.rst

doc/source/reference/random/parallel.rst

numpy/random/_bit_generator.pyx

rgommers · 2019-10-13T13:15:09Z

API and docs part of this PR look quite good to me now, module the few typos I just pointed out.

bashtage · 2019-10-13T20:12:21Z

I'm confused why numpy/random/generator.pyx → numpy/random/_generator.pyx has been marked private. Isn't this the main file that one would use to get low-level access to the C functions?

mattip · 2019-10-14T18:31:21Z

The public/private distinction is for python access. C and Cython access still need to be worked out, once we have solidified the python side, see #14604.

bashtage · 2019-10-16T14:29:01Z

Did you mean to commit numpy/random/_bounded_integers.pyx? The is a template-processed output from numpy/random/_bounded_integers.pyx.in.

bashtage · 2019-10-16T14:30:07Z

Same comment for numpy/random/_bounded_integers.pxd which is dervied from numpy/random/_bounded_integers.pxd.in

bashtage

Very small points. Only worthwhile ones are the commits of the pyx and pxd that come from templates.

bashtage · 2019-10-16T14:21:58Z

doc/source/reference/random/bit_generators/index.rst

-   PCG64 <pcg64>
-   Philox <philox>
-   SFC64 <sfc64>
+    MT19937 <mt19937>


Does 3 or 4 spaces matter? File seem to use both.

unified to 4, would be nice to do this gradually across the documentation

I think it only has to be consistent per file to work. Pretty sure the default is 3 spaces everywhere.

doc/source/reference/random/new-or-different.rst

bashtage · 2019-10-16T14:31:02Z

numpy/random/_bounded_integers.pyx.in


 np.import_array()

+cdef extern from "include/distributions.h":


Should these be in _bounded_integers.pxd.in?

I don't think so, then we would have to ship distributions.h. But I think we should revisit this in terms of documented use-cases in #14604 after this goes in.

numpy/random/tests/test_seed_sequence.py

rgommers · 2019-10-17T21:40:23Z

Okay this seems good to go now. Let's get this in so we can start looking at the API part. Thanks @mattip!

mattip added the 30 - API label Sep 27, 2019

mattip commented Sep 27, 2019

View reviewed changes

rgommers added 01 - Enhancement component: numpy.random labels Sep 28, 2019

mattip mentioned this pull request Sep 29, 2019

MAINT: align random double_fill to float_fill, simplify distributions.c #14611

Closed

mattip force-pushed the random-api branch 2 times, most recently from 082e17e to 724ec06 Compare October 11, 2019 12:06

mattip added 6 commits October 11, 2019 15:08

API: rearrange the cython files in numpy.random

e527e71

API: rename common, bounded_integers -> _common, _bounded_integers; c…

6fd7ec9

…leanup

API: make BitGenerators private

7e597f5

API: refactor function names in distribution.{h,c}, refactor float_fill

fbda484

API: remove unused functions from distributions.h

aeac7d5

API: move bit_generator and generator to be private

260bf67

mattip force-pushed the random-api branch from 724ec06 to 260bf67 Compare October 11, 2019 12:09

mattip changed the title ~~WIP, API: rearrange the cython files in numpy.random~~ API: rearrange the cython files in numpy.random Oct 11, 2019

rgommers reviewed Oct 12, 2019

View reviewed changes

doc/source/reference/random/new-or-different.rst Outdated Show resolved Hide resolved

mattip mentioned this pull request Oct 12, 2019

ENH: random: Add the multivariate hypergeometric distribution. #13794

Merged

mattip added this to the 1.18.0 release milestone Oct 12, 2019

DOC: clean up for moved references, remove ISeedSequence

185efe1

rgommers reviewed Oct 13, 2019

View reviewed changes

doc/source/reference/random/parallel.rst Outdated Show resolved Hide resolved

rgommers reviewed Oct 13, 2019

View reviewed changes

numpy/random/_bit_generator.pyx Outdated Show resolved Hide resolved

DOC: typos from review

fbf89d8

mattip mentioned this pull request Oct 15, 2019

Doc warnings #14699

Merged

bashtage reviewed Oct 16, 2019

View reviewed changes

TEST, DOC: fixes from review

09708c9

mattip requested a review from rgommers October 17, 2019 13:19

DOC: fix from review

1531642

rgommers merged commit 9ee262b into numpy:master Oct 17, 2019

This was referenced Oct 18, 2019

API: restructure and document numpy.random C-API #14604

Merged

API: provide examples of use of numpy random API via user stories #14778

Open

rgommers mentioned this pull request Dec 23, 2019

np.random.bit_generator broken at 1.18 #15152

Closed

mattip deleted the random-api branch October 13, 2020 13:54


		np.import_array()

		cdef extern from "include/distributions.h":

Uh oh!

Conversation

mattip commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Sep 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgommers commented Sep 28, 2019

Uh oh!

rgommers commented Sep 28, 2019

Uh oh!

mattip commented Sep 28, 2019

Uh oh!

mattip commented Oct 7, 2019

Uh oh!

rgommers commented Oct 7, 2019

Uh oh!

mattip commented Oct 8, 2019

Uh oh!

mattip commented Oct 11, 2019

Uh oh!

rgommers commented Oct 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgommers commented Oct 13, 2019

Uh oh!

bashtage commented Oct 13, 2019

Uh oh!

mattip commented Oct 14, 2019

Uh oh!

bashtage commented Oct 16, 2019

Uh oh!

bashtage commented Oct 16, 2019

Uh oh!

bashtage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgommers commented Oct 17, 2019

Uh oh!

Reviewers

mattip commented Sep 27, 2019 •

edited

Loading