ENH: supply our version of numpy.pxd, requires cython>=0.29 by mattip · Pull Request #12284 · numpy/numpy

mattip · 2018-10-29T11:47:54Z

Adds our own numpy.pxd, copied from the cython __init__.pxd. It will only be used if the numpy include path from numpy.get_includes() is added to the cython compile flags, otherwise the cython one will still be used.

Modifications were needed to mtrand.pyx since ndarray.shape is a C npy_intp in the newer numpy.pxd, not a python-level sequence attribute.

mattip · 2018-10-29T11:55:06Z

Also the numpy.pxd uses the Cython check_size ignore syntax to avoid the "size changed" warnings, which requires cython 0.29 and above. Once downstream users update, we can remove the warning filters from numpy.__init__.py. xref #11788

eric-wieser · 2018-10-29T16:09:36Z

numpy/core/include/numpy/numpy.pxd

Given we just added this, is there any reason to keep the deprecated headers?

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.

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)

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.

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 sufficient.

mattip · 2018-10-30T07:26:47Z

Builds are failing since LGTM and the CHROOT travis build are not using Cython 0.29

mattip · 2018-10-30T12:23:11Z

xref cython/cython#2498

mattip · 2018-10-31T14:50:36Z

after cherry picking the changes from #12299, travis and lgtm are now passing

.travis.yml

eric-wieser · 2018-11-18T05:59:51Z

numpy/random/mtrand/mtrand.pyx

Is the fact that this is missing from the Python.pxi an upstream bug?

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.

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/random/mtrand/mtrand.pyx

eric-wieser · 2018-11-18T06:05:10Z

numpy/core/include/numpy/numpy.pxd

this function worries me - can we not reuse our PEP3118 format string builder, rather than having a second implementation?

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()?

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

jdemeyer · 2019-03-24T19:12:49Z

I don't understand why you hide numpy.pxd in the include subdirectory.

Typically, the layout of .pxd files follows that of the Python project. Think of a cimport as analogous to an import. So, if you expect users to do cimport numpy, then it makes more sense to move numpy.pxd to a top-level __init__.pxd file (more or less the way it is in Cython).

mattip · 2019-03-24T19:31:54Z

@jdemeyer whatever works correctly. I am trying to get this to work with pandas, so that python setup.py build_ext --inplace --force will find the numpy pxd before the one in Cython/Include/numpy/__init__.pxd. It seems no matter what I do the cython one is found first (I added a line like x = xxxxxx in the file to make compilation fail if the wrong one is chosen). What I tried:

using the include_path = pkg_resources.resource_filename('numpy', 'core/include')
putting it in site-packages/numpy/__init__.pxd (your suggestion)

Neither of those helped. But just now I tried using include_path = pkg_resources.resource_filename('numpy', 'core/include/numpy') which seems to work

seberg

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).

mattip · 2019-09-03T16:31:30Z

rebased off master
fixed release note: no changes in include path are needed, the pxd file is picked up via cimport numpy after BUG: prefer sys.path before Cython/Include when searching for pxd files cython/cython#2910 which has been backported to the current release

seberg · 2019-09-03T16:51:39Z

doc/release/upcoming_changes/12284.new_feature.rst

Suggested change

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).

fixed, thanks

seberg · 2019-09-03T19:49:20Z

Is there a difference between this pyd and the one in cython that can explain the windows crash? Or is that a random crash due to OpenBLAS or so?

mattip · 2019-09-04T05:38:43Z

seems like a random crash

seberg · 2019-09-04T15:11:32Z

Thanks, lets put this in then.

jakirkham · 2019-09-04T23:52:13Z

Thanks all for the work on this! 😀

scoder · 2020-05-05T17:53:08Z

@mattip I checked in my local numpy 1.18.3 installation and it lacks the numpy/__init__.pxd file that this PR added. My guess is that it still needs to be added to the distutils package_data in order to be included not just in the sdist but also in binary distributions.

scoder · 2020-05-05T18:02:39Z

Although … the .pxd files in the numpy/random/ directory are included. So, why it numpy/__init__.pxd not in my wheel?

jakirkham · 2020-05-05T18:04:57Z

Not sure. Looks like they should all be there.

scoder · 2020-05-05T18:10:44Z

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 setup.py. So how do the files in numpy.random get included?

scoder · 2020-05-05T18:13:25Z

Ah, found it, I think.

mattip · 2020-05-05T18:14:43Z

@scoder gh-16162, thanks for the heads up.

scoder · 2020-05-05T20:13:59Z

numpy/__init__.pxd

+        # 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


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. :)

Cython first. I think there are tests to verify that the buffer protocol works for ndarray in cython.

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.

I removed both methods in cython/cython@b794997

removing here too

…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

jbrockmendel · 2020-05-12T20:31:14Z

numpy/__init__.pxd

+    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)


@seberg I think this is inaccurate xref https://stackoverflow.com/questions/28184211/calling-pyarray-searchsorted-from-cython-3-or-4-arguments

Hmm, looks like you are right. Since 2012 it has been

PyArray_SearchSorted(ndarray, object, NPY_SEARCHSIDE, object)

Was it safe to add an argument when its NULL?! Or was this just always wrong?

mattip added 01 - Enhancement component: numpy.random component: build 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels Oct 29, 2018

eric-wieser reviewed Oct 29, 2018

View reviewed changes

This was referenced Oct 29, 2018

BUILD: force LGTM to use cython>=0.29 #12288

Merged

ENH: Do not emit compiler warning if forcing old API #12286

Merged

mattip force-pushed the numpy.pxd branch 2 times, most recently from 88b8d15 to 4068465 Compare October 31, 2018 08:26

charris reviewed Nov 16, 2018

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

mattip force-pushed the numpy.pxd branch from b172e77 to 9ce1268 Compare November 18, 2018 05:56

eric-wieser reviewed Nov 18, 2018

View reviewed changes

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

eric-wieser reviewed Nov 18, 2018

View reviewed changes

mattip changed the title ~~ENH: supply our version of numpy.pxd, requires cython>=0.29~~ WIP: ENH: supply our version of numpy.pxd, requires cython>=0.29 Nov 18, 2018

mattip mentioned this pull request Nov 26, 2018

add method to silence old API deprecation build warnings #11653

Closed

mattip force-pushed the numpy.pxd branch from d620901 to 51eed80 Compare December 3, 2018 20:09

mattip force-pushed the numpy.pxd branch 3 times, most recently from ad1629f to 29ae6b0 Compare January 1, 2019 08:26

mattip mentioned this pull request Mar 21, 2019

ENH: randomgen #13163

Merged

mattip force-pushed the numpy.pxd branch from 29ae6b0 to 3dadf5d Compare March 21, 2019 14:03

mattip removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 21, 2019

jdemeyer mentioned this pull request Mar 24, 2019

Remove Cython/Includes/Deprecated cython/cython#2904

Merged

mattip force-pushed the numpy.pxd branch from e89e224 to 0158cea Compare August 18, 2019 22:55

mattip removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Aug 19, 2019

mattip requested a review from seberg August 19, 2019 11:35

rgommers added this to the 1.18.0 release milestone Aug 28, 2019

seberg approved these changes Sep 3, 2019

View reviewed changes

mattip force-pushed the numpy.pxd branch from 065c11c to 2ba3bd9 Compare September 3, 2019 16:28

mattip closed this Sep 3, 2019

mattip reopened this Sep 3, 2019

seberg reviewed Sep 3, 2019

View reviewed changes

ENH: supply our version of numpy.pxd, adjust as needed

2b250b0

mattip force-pushed the numpy.pxd branch from 2ba3bd9 to 2b250b0 Compare September 3, 2019 22:57

seberg merged commit 3883be3 into numpy:master Sep 4, 2019

mattip mentioned this pull request May 5, 2020

Migrate NumPy declarations to the NumPy project cython/cython#3573

Closed

mattip mentioned this pull request May 5, 2020

BUG: add missing numpy/__init__.pxd to the wheel #16162

Merged

scoder reviewed May 5, 2020

View reviewed changes

jbrockmendel reviewed May 12, 2020

View reviewed changes

charris mentioned this pull request May 12, 2020

The PyArray_SearchSorted function has wrong signature in __init__.pxd #16219

Closed

mattip deleted the numpy.pxd branch June 8, 2020 06:58

	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``

Uh oh!

Conversation

mattip commented Oct 29, 2018

Uh oh!

mattip commented Oct 29, 2018

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!

mattip commented Oct 30, 2018

Uh oh!

mattip commented Oct 30, 2018

Uh oh!

mattip commented Oct 31, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Mar 24, 2019

Uh oh!

mattip commented Mar 24, 2019

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

mattip commented Sep 3, 2019

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Sep 3, 2019

Uh oh!

mattip commented Sep 4, 2019

Uh oh!

seberg commented Sep 4, 2019

Uh oh!

jakirkham commented Sep 4, 2019

Uh oh!

scoder commented May 5, 2020

Uh oh!

scoder commented May 5, 2020

Uh oh!

jakirkham commented May 5, 2020

Uh oh!

scoder commented May 5, 2020

Uh oh!

scoder commented May 5, 2020

Uh oh!

mattip commented May 5, 2020

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

eric-wieser Nov 18, 2018 •

edited

Loading

seberg Sep 3, 2019 •

edited

Loading