-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Added missing np.import_array() #14813
Conversation
In principle this should always be called before using cimported Numpy from Cython files. It becomes more important on recent versions of Cython (since they use the Numpy API rather than directly looking into the objects)
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.
Do we want to remove the Cython version pin from gh-14801 here so we can see the CI pass with latest Cython alpha? Is this meant to guard against the issue on the latest alpha?
IIUC the related NumPy PR will not solve the alpha release problem: numpy/numpy#20042
Yes - that's probably a good idea to show that it genuinely has fixed what I believe it has.
I think it would solve it if merged but it's probably best fixed here and in Cython. |
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.
Here are some more files in stats that I think need np.import_array()
:
- scipy/stats/_qmc_cy.pyx
- scipy/stats/biasedurn.pyx.templ
- scipy/stats/_unuran/unuran_wrapper.pyx.templ
scipy/stats/_qmc_cy.pyx has it already Line 12 in 0785d6f
I missed templ files (and a few other auto-generated files) when I did the search so I've added them now - thanks |
So, am I understanding this correctly: from now on, any A user in me believes there are reasons why Cython does not do it automatically, but then is it documented somewhere? It should be --- this can possibly trip quite a bit of one-off small-script downstream user code. |
In principle this was always the case. Cython 3 changes to how things like
Cython 3 actually does try to do it automatically if the user forgets and emits a reminder warning. A recent change in Numpy broke that (although it should now be fixed in Cython). I can't find it in the Cython documentation though, which looks like an omission. Essentially the Cython view is that users should call |
Thanks @da-woods , this is helpful. So a user recommendation used to be "import_array when using PyArray_... functions directly", now it's "always import_array". While this is OT for this PR (and the PR LGTM), I'd ask to document this as thorough as realistic on the Cython side. I'm on to updating my teaching materials and small projects where I contributed Cython bits and pieces :-). |
Thanks @da-woods. So if I got this correctly, if we want to use NumPy in Cython, the canonical way should be to add
Is that right? I just got to play with Cython while working on the QMC module so I am really new to these. |
Yes that's right. You only need |
Followed up on the cython-users mailing list, here's a link for completeness https://groups.google.com/g/cython-users/c/6qGmzvIv7Io |
* Added missing np.import_array() In principle this should always be called before using cimported Numpy from Cython files. It becomes more important on recent versions of Cython (since they use the Numpy API rather than directly looking into the objects) * Remove version pin * Added some missing lines in templ files (cherry picked from commit 2f4bd42)
In principle this should always be called before using cimported
Numpy from Cython files. It becomes more important on recent
versions of Cython (since they use the Numpy API rather than
directly looking into the objects)
Reference issue
#14732
What does this implement/fix?
Ensures that
np.import_array()
is always called any-time thatnumpy
iscimported