ENH: supply our version of numpy.pxd, requires cython>=0.29 #12284
Conversation
|
Also the |
numpy/core/include/numpy/numpy.pxd
Outdated
| NPY_SEARCHRIGHT | ||
|
|
||
| enum: | ||
| # DEPRECATED since NumPy 1.7 ! Do not use in new code! |
There was a problem hiding this comment.
Given we just added this, is there any reason to keep the deprecated headers?
There was a problem hiding this comment.
As long as we have npy_1_7_deprecated_api.h for people who are still using the old API (scipy, pandas, ...), we should reflect old values in Cython.
There was a problem hiding this comment.
In order to truly deprecate the old API, we need either cython/cython#2640 or to revert the attribute hiding PyArray_FUNCTIONS since cython does not convert python attribute lookup array.ndim into a getter function PyArray_NDIMS(array)
There was a problem hiding this comment.
Why do we need array.ndim to be converted into PyArray_NDIMS(array)? It would be good to have written down somewhere what problem is trying to be solved here.
There was a problem hiding this comment.
cython class overview gives the motivation: write python-like code, run it through the cython compiler and get efficient C code out. Extension type spec dives into the details of how to map python object attributes to C struct fields.
We try to hide the C struct fields in PyArrayObject_fields behind C getter functions to the opaque PyArrayObject struct, so the techniques described in the links above are not suffi
8000
cient.
|
Builds are failing since LGTM and the CHROOT travis build are not using Cython 0.29 |
|
xref cython/cython#2498 |
88b8d15 to
4068465
Compare
|
after cherry picking the changes from #12299, travis and lgtm are now passing |
numpy/random/mtrand/mtrand.pyx
Outdated
| @@ -35,6 +33,17 @@ cdef extern from "math.h": | |||
| double sin(double x) | |||
| double cos(double x) | |||
|
|
|||
| # Put initarray first since it includes the NPY_NO_DEPRECATED_API definition | |||
| cdef extern from "Python.h": | |||
| ctypedef int Py_intptr_t | |||
There was a problem hiding this comment.
Is the fact that this is missing from the Python.pxi an upstream bug?
There was a problem hiding this comment.
which Python.pxi? The cdef extern from "Python.h" tells Cython to add #include Python.h at this point in the output C file, and promises that that header file (or one of the headers included by it) will include a typedef int Py_intptr_t statement.
There was a problem hiding this comment.
this one - my assumption was that this header already includes all of the cdef externs that match to Python.h. Is that not true?
Edit: Seems that's our own file
numpy/core/include/numpy/numpy.pxd
Outdated
| else: | ||
| return () | ||
|
|
||
| cdef inline char* _util_dtypestring(dtype descr, char* f, char* end, int* offset) except NULL: |
There was a problem hiding this comment.
this function worries me - can we not reuse our PEP3118 format string builder, rather than having a second implementation?
There was a problem hiding this comment.
hmm. _buffer_format_string is buried pretty deep inside the code. I wonder what the performance hit would be if we went via PyObject_GetBuffer()?
There was a problem hiding this comment.
I switched this all for PyObject_GetBuffer() which calls array_getbuffer, and a NOOP __release_buffer__, with a note explaining why it is a NOOP currently.
At some point if we drop our _buffer_info_cache we should revisit this code
ad1629f to
29ae6b0
Compare
|
I don't understand why you hide Typically, the layout of |
|
@jdemeyer whatever works correctly. I am trying to get this to work with pandas, so that
Neither of those helped. But just now I tried using |
There was a problem hiding this comment.
It seems right to me to move it into numpy (and I think there is general consensus here). Since it moved around, just to be sure: numpy.get_includes() does not need to be modified for dependencies to pick up our numpy.pxd file?
Unfortunately, there are some merge conflicts (I can fix them up maybe though).
|
|
|
||
| Add our own ``*.pxd`` cython import file | ||
| -------------------------------------------- | ||
| Added a ``numpy/__init__.pxd`` file. It will be used for `cpython import numpy` |
There was a problem hiding this comment.
| Added a ``numpy/__init__.pxd`` file. It will be used for `cpython import numpy` | |
| Added a ``numpy/__init__.pxd`` file. It will be used for ``cimport numpy as cnp`` |
is what you mean I think? And I am not sure we still need the search path link (at least not as a link).
|
Is there a difference between this |
|
seems like a random crash |
|
Thanks, lets put this in then. |
|
Thanks all for the work on this! 😀 |
|
@mattip I checked in my local numpy 1.18.3 installation and it lacks the |
|
Although … the |
|
Not sure. Looks like they should all be there. |
Yes, that's the MANIFEST file that's used for the sdist. It doesn't have an effect on the binary distributions. However, I couldn't find any package data configuration or so in NumPy's |
|
Ah, found it, I think. |
| # Note: This syntax (function definition in pxd files) is an | ||
| # experimental exception made for __getbuffer__ and __releasebuffer__ | ||
| # -- the details of this may change. | ||
| def __getbuffer__(ndarray self, Py_buffer* info, int flags): | ||
| PyObject_GetBuffer(<object>self, info, flags); | ||
|
|
||
| def __releasebuffer__(ndarray self, Py_buffer* info): | ||
| # We should call a possible tp_bufferrelease(self, info) but no | ||
| # interface to that is exposed by cython or python. And currently | ||
| # the function is NULL in numpy, we rely on refcounting to release | ||
| # info when self is collected | ||
| pass |
There was a problem hiding this comment.
NumPy can just leave these out entirely. They were added in the time when Python 2.6/3.0 were still new and the average NumPy installation on user side didn't support the PEP 3118 buffer protocol yet. The times have changed a bit since then. Even Cython can probably drop these by now. :)
There was a problem hiding this comment.
Cython first. I think there are tests to verify that the buffer protocol works for ndarray in cython.
There was a problem hiding this comment.
The buffer protocol does work for ndarrays, because NumPy supports it natively. The implementation through PyObject_GetBuffer() shows this clearly (Cython still uses a real one). In fact, since __releasebuffer__ is empty, releasing buffers will not even work here.
There was a problem hiding this comment.
I removed both methods in cython/cython@b794997
…umPy 'ndarray' since there are probably no NumPy installations out there anymore that do not support the buffer protocol themselves and are still worth supporting. See numpy/numpy#12284 (comment) See GH-3573
| object PyArray_Choose (ndarray, object, ndarray, NPY_CLIPMODE) | ||
| int PyArray_Sort (ndarray, int, NPY_SORTKIND) | ||
| object PyArray_ArgSort (ndarray, int, NPY_SORTKIND) | ||
| object PyArray_SearchSorted (ndarray, object, NPY_SEARCHSIDE) |
There was a problem hiding this comment.
@seberg I think this is inaccurate xref https://stackoverflow.com/questions/28184211/calling-pyarray-searchsorted-from-cython-3-or-4-arguments
There was a problem hiding this comment.
Hmm, looks like you are right. Since 2012 it has been
PyArray_SearchSorted(ndarray, object, NPY_SEARCHSIDE, object)
There was a problem hiding this comment.
Was it safe to add an argument when its NULL?! Or was this just always wrong?
Fixes #11803
Adds our own
numpy.pxd, copied from the cython__init__.pxd. It will only be used if the numpy include path fromnumpy.get_includes()is added to the cython compile flags, otherwise the cython one will still be used.Modifications were needed to
mtrand.pyxsincendarray.shapeis a Cnpy_intpin the newernumpy.pxd, not a python-level sequence attribute.