Skip to content
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

Merged
merged 3 commits into from Oct 6, 2021
Merged

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Oct 5, 2021

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 that numpy is cimported

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)
Copy link
Contributor

@tylerjereddy tylerjereddy left a 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

@github-actions github-actions bot added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label Oct 5, 2021
@da-woods
Copy link
Contributor Author

da-woods commented Oct 5, 2021

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?

Yes - that's probably a good idea to show that it genuinely has fixed what I believe it has.

IIUC the related NumPy PR will not solve the alpha release problem: numpy/numpy#20042

I think it would solve it if merged but it's probably best fixed here and in Cython.

Copy link
Member

@tirthasheshpatel tirthasheshpatel left a 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

@da-woods
Copy link
Contributor Author

da-woods commented Oct 6, 2021

scipy/stats/_qmc_cy.pyx has it already

np.import_array()

I missed templ files (and a few other auto-generated files) when I did the search so I've added them now - thanks

@ev-br
Copy link
Member

ev-br commented Oct 6, 2021

So, am I understanding this correctly: from now on, any cimport numpy as np will need to be followed by np.import_array() ?

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.

@da-woods
Copy link
Contributor Author

da-woods commented Oct 6, 2021

So, am I understanding this correctly: from now on, any cimport numpy as np will need to be followed by np.import_array() ?

In principle this was always the case. Cython 3 changes to how things like .shape are accessed makes it much more sensitive to this though (since they now use the imported Numpy API instead of looking directly in the C struct)

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.

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 import_array and probably should always have been calling it. However, we do make a best-effort attempt to do it ourselves.

@ev-br
Copy link
Member

ev-br commented Oct 6, 2021

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

@tupui
Copy link
Member

tupui commented Oct 6, 2021

Thanks @da-woods. So if I got this correctly, if we want to use NumPy in Cython, the canonical way should be to add

import numpy as np
cimport numpy as np  # and sometimes I see this as cnp. I have no clue what I should do
np.import_array()

Is that right? I just got to play with Cython while working on the QMC module so I am really new to these.

@da-woods
Copy link
Contributor Author

da-woods commented Oct 6, 2021

Thanks @da-woods. So if I got this correctly, if we want to use NumPy in Cython, the canonical way should be to add

import numpy as np
cimport numpy as np  # and sometimes I see this as cnp. I have no clue what I should do
np.import_array()

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. np vs cnp is fine either way - it's just personal choice. I think some people find it clearer to use a different name for the runtime and compile time definitions.

You only need cimport numpy (and np.import_array()) if you're planning to use "Cython features" with Numpy (for example, typing something as np.ndarray). If the code would work unchanged in Python then you only need the import

@tylerjereddy tylerjereddy added this to the 1.8.0 milestone Oct 6, 2021
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Oct 6, 2021
@tylerjereddy tylerjereddy merged commit 2f4bd42 into scipy:master Oct 6, 2021
@tylerjereddy
Copy link
Contributor

Sounds like @ev-br is ok with this PR as well, and the other queries here are just us learning about Cython I think.

Squash-merged and marked for backport, thanks for engaging from the Cython side @da-woods.

@ev-br
Copy link
Member

ev-br commented Oct 6, 2021

Followed up on the cython-users mailing list, here's a link for completeness https://groups.google.com/g/cython-users/c/6qGmzvIv7Io

rgommers pushed a commit to rgommers/scipy that referenced this pull request Oct 29, 2021
* 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)
@tylerjereddy tylerjereddy modified the milestones: 1.8.0, 1.7.2 Oct 29, 2021
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants