ENH: expose `bit_generator` and random C-API to cython #15463

mattip · 2020-01-29T06:46:48Z

As pointed out in the comment by @jamesthomasgriffin, we did not include a pxd file to expose the distribution functions documented in the random c-api. This PR adds a c_distributions.pxd file that exposes them.

~~A test is still needed that this all works as advertised. Could someone familiar with typical use please suggest one?~~ Complicated test to build a cython c-extension module added.

mattip · 2020-01-29T18:40:44Z

Just a pxd file is not enough. We need to ship either the source for these functions or a so/dll+lib that users can link to. Shipping the source is complicated, but so is the other option. @rkern @bashtage ideas?

bashtage · 2020-01-29T18:54:11Z

Nothing other than what you say. The "first best" would be to ship a lib/so that could be linked to like npymath.lib/so, e.g., npyrandom.lib/so (or possibly to stick them in npymath if possible).

mattip · 2020-01-29T21:19:50Z

the npymath parallel looks good. I will try that, thanks

…-API

numpy/random/_examples/cython/extending_distributions.pyx

bashtage · 2020-01-31T14:29:23Z

Looks like a good and surprisingly simple solution.

numpy/random/_examples/cython/extending_distributions.pyx

mattip · 2020-02-01T20:19:11Z

@rkern I don't understand the test failure in test_random.py 1585 in the threaded tests. on windows. Any thoughts?

numpy/random/setup.py

mattip · 2020-02-02T12:44:35Z

numpy/random/setup.py

@@ -52,78 +54,87 @@ def generate_libraries(ext, build_dir):
    PCG64_DEFS = []
    # One can force emulated 128-bit arithmetic if one wants.
    #PCG64_DEFS += [('PCG_FORCE_EMULATED_128BIT_MATH', '1')]
+    depends = ['__init__.pxd', 'c_distributions.pxd', '_bit_generator.pxd']


_bit_generator.pxd existed in 1.18, but was not copied into the wheel. The use of depends on line 137 should fix that.

mattip · 2020-02-02T20:06:20Z

apparently something in this PR broke windows. Maybe the npyrandom.lib is off somehow

eric-wieser · 2020-02-02T20:33:15Z

numpy/random/_examples/cython/extending_distributions.pyx

+    # Cast the pointer
+    rng = <bitgen_t *> PyCapsule_GetPointer(capsule, capsule_name)
+    randoms = np.empty(n, dtype=_dtype)
+    if _dtype is np.float32:


Suggested change

if _dtype is np.float32:

if _dtype.type is np.float32:

or

Suggested change

if _dtype is np.float32:

if _dtype == np.float32:

As written this is always false. The former is more consistent with line 100.

Got be careful here, the latter is correct. The former would have to also check _dtype.isnative. Right now dtype=">f8" (on little endian machines) is probably broken!

However, I think I actually like the dtype.type is np.float32 and dtype.isnative spelling I think.

EDIT: I guess here the isnative check needs to be further up anyway though, maybe just create an issue for it as a separate thing.

This is meant as a test-run for something to replace the key = np.dtype(dtype).name in _generator.pyx and mtrand.pyx so it would be good to get it right here.

I think dtype == np.float32 is probably simplest, but make sure to update line 100 too.

True dtype == ... is shortest. May be worth to time it in case the type + isnative happens to be much faster (right now).
I timed it, and it:

In [2]: %timeit dt == np.float64 85 ns ± 5.03 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) In [3]: %timeit dt.type is np.float64 57.3 ns ± 0.108 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) In [4]: %timeit dt.type is np.float64 and dt.isnative 81.6 ns ± 0.697 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

so since the == includes the isnative check there is no speed difference between the spellings , so probably should use the == one. The only thing we could consider with respect to fixing the speed issue of dtype.name check would be to add a fast-path for the very common ~~dtype~~ default.

using the == form

Still seems to be the == form here?

numpy/random/_examples/cython/extending_distributions.pyx

mattip · 2020-02-03T15:16:08Z

"Dogfooding" our code is helpful. The use of the npyrandom library internally and for the C-API cython test showed that the RAND_INT_TYPE macro in the header files is confusing (well, it confused me). I moved it out of the header and duplicated a header file with the long alternative. It should now be more likely to get a compilation error rather than runtime crashes.

mattip · 2020-02-03T15:19:22Z

numpy/random/mtrand.pyx

        double r
        double q
        double fm
-        int64_t m
+        long m
        double p1
        double xm
        double xl


cython doesn't seem to care what int is used in the struct, it is later filled out by the compiler. But cleaning this up anyway for consistency.

mattip · 2020-02-03T15:21:05Z

numpy/random/src/distributions/distributions.c

 #include "ziggurat_constants.h"
 #include "logfactorial.h"

 #if defined(_MSC_VER) && defined(_WIN64)
-#include <intrin.h>
+  #include <intrin.h>
 #endif

 /* Inline generators for internal use */


This file is formatted with 2-space tabs instead of 4-space. Is now a good time to remedy that?

I am happy if you do that (just ping Kevin on it before merging?), should be its own pure STY PR though (and if possible cover other files in random if they exist?). Not sure how others feel about such fixups, but I tend to feel that not doing style cleanups is also not a solution...

rkern · 2020-02-19T16:01:21Z

+1 to move _bit_generator to bit_generator.

mattip · 2020-02-19T16:04:56Z

@r-devulap the reciprical strided test is failing here as well

r-devulap · 2020-02-19T23:26:10Z

Hmm, did that test failure vanish? @mattip

mattip · 2020-02-19T23:33:44Z

I reran the failing CI run, and it dissappeared. But see gh-15610

mattip · 2020-02-24T11:17:42Z

CI is passing after the _bit_generator -> bit_generator rename

mattip · 2020-02-25T18:34:15Z

Added a release note, CI is still green.

seberg

Thanks for the ping Matti. LGTM, the is part should be fixed in the example but everything else we can do later I think, so lets keep things moving.

seberg · 2020-03-16T18:44:28Z

numpy/random/_examples/cython/extending_distributions.pyx

+    # Cast the pointer
+    rng = <bitgen_t *> PyCapsule_GetPointer(capsule, capsule_name)
+    randoms = np.empty(n, dtype=_dtype)
+    if _dtype is np.float32:


Still seems to be the == form here?

seberg · 2020-03-16T18:46:51Z

numpy/random/setup.py

+        install_dir='lib',
+        build_info={
+            'include_dirs' : [],  # empty list required for creating npyrandom.h
+            'extra_compiler_args' : (['/GL-'] if is_msvc else []),


Would it make sense to comment on why global optimization is disabled? (and maybe that it is global optimization?)

I copied it from numpy/core/setup.py

seberg · 2020-03-16T18:51:37Z

Sould the release notes in the same or different fragment include that c_distributions.pxd is now exposed, it is, right?

mattip · 2020-03-16T21:01:22Z

Mentioned the c_distributions.pxd in the release note.

seberg · 2020-03-16T21:15:39Z

numpy/random/_examples/cython/extending_distributions.pyx

@@ -106,7 +106,7 @@ def uniforms_ex(bit_generator, Py_ssize_t n, dtype=np.float64):
    # Cast the pointer
    rng = <bitgen_t *> PyCapsule_GetPointer(capsule, capsule_name)
    randoms = np.empty(n, dtype=_dtype)
-    if _dtype is np.float32:
+    if _dtype == np.float32:


Sorry, I did not look at it careful enough again right now. This is still incorrect for non-native byte order, would need to add an error branch and elif _dtype == np.float64. On the up-side, you can just move the above raise TypeError('Unsupported dtype "%r"' % dtype) in the else branch then, so that it is nicer anyway.
I can do that if you are getting tired of this :).

seberg · 2020-03-16T22:01:11Z

Thanks, I renamed it the title, hope that fits well enough (if not, can change it in the PR title at least, will squash merge).

charris · 2020-04-06T15:43:11Z

@mattip Hmm, I missed this for the 1.18.1 release (wrong milestone) and it may be a bit late to put it in the 1.18.3 release. It doesn't seem right to add new functionality so late in the release cycle.

bashtage · 2020-04-06T15:45:28Z

I think there is no need to rush is out since it is an enhancement and not a bug. 1.19 seems like a good choice..

charris · 2020-04-06T15:52:57Z

@bashtage OK, let's go with that.

rkern · 2021-08-04T14:54:45Z

@charris @bashtage Can we backport at least the np.random.bit_generator rename to a 1.18.6 bugfix release. This actually is a bug, not an enhancement; we always wanted people to be able to import BitGenerator. It seems to be causing issues with scipy.

charris · 2021-08-04T15:19:19Z

We could try, the wheels code may have bitrotted, making things more difficult. Want to make a PR?

mattip added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported component: numpy.random labels Jan 29, 2020

mattip added 2 commits January 31, 2020 16:18

BUG: add missing c_distributions.pxd to enable cython use of random C…

5094692

…-API

ENH, TST: add npyrandom library like npymath, test cython use of it

db01d81

mattip force-pushed the c_distributions branch from 751d849 to db01d81 Compare January 31, 2020 14:24

mattip commented Jan 31, 2020

View reviewed changes

numpy/random/_examples/cython/extending_distributions.pyx Show resolved Hide resolved

BUG: actually prefix f-string with f

93ec29d

eric-wieser reviewed Jan 31, 2020

View reviewed changes

numpy/random/_examples/cython/extending_distributions.pyx Outdated Show resolved Hide resolved

eric-wieser reviewed Jan 31, 2020

View reviewed changes

numpy/random/_examples/cython/extending_distributions.pyx Outdated Show resolved Hide resolved

mattip force-pushed the c_distributions branch from e9d27e7 to af317e7 Compare February 2, 2020 10:46

MAINT: fixes from review, add _bit_generato_bit_generator.pxd

f66326a

mattip force-pushed the c_distributions branch from af317e7 to f66326a Compare February 2, 2020 12:40

mattip commented Feb 2, 2020

View reviewed changes

numpy/random/setup.py Show resolved Hide resolved

mattip commented Feb 2, 2020

View reviewed changes

eric-wieser reviewed Feb 2, 2020

View reviewed changes

numpy/random/_examples/cython/extending_distributions.pyx Outdated Show resolved Hide resolved

eric-wieser reviewed Feb 2, 2020

View reviewed changes

numpy/random/_examples/cython/extending_distributions.pyx Show resolved Hide resolved

mattip commented Feb 3, 2020

View reviewed changes

mattip mentioned this pull request Feb 4, 2020

BUG: Large overhead in some random functions #15460

Closed

mattip requested a review from eric-wieser February 4, 2020 15:30

mattip added the triage review Issue/PR to be discussed at the next triage meeting label Feb 12, 2020

MAINT, API: move _bit_generator to bit_generator

8c6a68b

mattip force-pushed the c_distributions branch from 2fc146f to 8c6a68b Compare February 24, 2020 09:46

DOC: add release note about moving bit_generator

ce13211

mattip requested a review from seberg March 15, 2020 21:17

seberg approved these changes Mar 16, 2020

View reviewed changes

DOC, MAINT: fixes from review

a111795

seberg reviewed Mar 16, 2020

View reviewed changes

MAINT: redo dtype determination from review

74ac4cf

seberg changed the title ~~BUG: add missing c_distributions.pxd, enables cython use of random C-API~~ ENH: expose bit_generator and random C-API to cython Mar 16, 2020

seberg merged commit 4f2b219 into numpy:master Mar 16, 2020

charris modified the milestones: 1.19.0 release, 1.18.3 release Apr 6, 2020

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Apr 6, 2020

charris removed this from the 1.18.3 release milestone Apr 6, 2020

chrisb83 mentioned this pull request Oct 21, 2020

ENH: improve ARGUS rv generation in scipy.stats scipy/scipy#11954

Merged

mattip deleted the c_distributions branch November 2, 2020 08:27

ENH: expose bit_generator and random C-API to cython #15463

ENH: expose bit_generator and random C-API to cython #15463

Conversation

mattip commented Jan 29, 2020 • edited Loading

mattip commented Jan 29, 2020

bashtage commented Jan 29, 2020

mattip commented Jan 29, 2020

bashtage commented Jan 31, 2020

mattip commented Feb 1, 2020

Choose a reason for hiding this comment

mattip commented Feb 2, 2020

eric-wieser Feb 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg Feb 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattip commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkern commented Feb 19, 2020

mattip commented Feb 19, 2020

r-devulap commented Feb 19, 2020

mattip commented Feb 19, 2020

mattip commented Feb 24, 2020

mattip commented Feb 25, 2020

seberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Mar 16, 2020

mattip commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seberg commented Mar 16, 2020

charris commented Apr 6, 2020

bashtage commented Apr 6, 2020

charris commented Apr 6, 2020

rkern commented Aug 4, 2021

charris commented Aug 4, 2021

ENH: expose `bit_generator` and random C-API to cython #15463

ENH: expose `bit_generator` and random C-API to cython #15463

mattip commented Jan 29, 2020 •

edited

Loading

eric-wieser Feb 2, 2020 •

edited

Loading

seberg Feb 2, 2020 •

edited

Loading

seberg Feb 3, 2020 •

edited

Loading