NF: Call `cnp.import_array()` explicitly to use the NumPy C API

dipy / dipy

DIPY is the paragon 3D/4D+ imaging library in Python. Contains generic methods for spatial normalization, signal processing, machine learning, statistical analysis and visualization of medical images. Additionally, it contains specialized methods for computational anatomy including diffusion, perfusion and structural imaging.

https://dipy.org

Other

688 stars 431 forks source link

NF: Call `cnp.import_array()` explicitly to use the NumPy C API #3266

Closed jhlegarreta closed 1 week ago

jhlegarreta commented 3 weeks ago

Call cnp.import_array() explicitly to use the NumPy C API: Cython modules (or anything using the NumPy C API) must call cnp.import_array() to import the NumPy C API.

Fixes:

  File "vector_fields.pyx", line 1, in init dipy.align.vector_fields
ImportError: numpy.core.multiarray failed to import
 (auto-generated because you didn't call 'numpy.import_array()' after
 cimporting numpy; use '<void>numpy._import_array' to disable if you
 are certain you don't need it).

reported by third-party tool (e.g. MNE) maintaners when testing their tools with NumPy 2.0.

Fixes #3265.

jhlegarreta commented 3 weeks ago

We need to add NumPy 2.0 to the stack of tested dependencies.

jhlegarreta commented 3 weeks ago

OK, all are failing. Should be compatible with NumPy<2.0.0, e.g. has been there since a very long time ago (to the oldest record that I have been able to access): https://github.com/numpy/numpy/commit/e9e0a8c32e72d739e4bb637bc85bb2b5a42545c7

I've seen this: https://github.com/pandas-dev/pandas/blob/main/pyproject.toml#L9-L11 https://github.com/pandas-dev/pandas/blob/main/pyproject.toml#L30-L31

Also in scikit-image: https://github.com/scikit-image/scikit-image/blob/main/pyproject.toml#L56 https://github.com/scikit-image/scikit-image/blob/main/pyproject.toml#L29

Additionally, here they mention that PyArray_ImportNumPyAPI is preferred over import_array: https://numpy.org/doc/stable/release/2.0.0-notes.html#new-c-api-import-functions

So not sure about the way forward.

jhlegarreta commented 2 weeks ago

Well, does not look like the build-system change worked in cd38e8d.

jhlegarreta commented 2 weeks ago

So remaining errors:

Not sure the approach to solve this: ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject, but Python 3.12 builds are not reporting those errors, probably thanks to 7b61aff6663b51c518245a5e3d3217b1583ab16a and possibly a5e6f26784460170b1814f10d732212d456fec48. Other errors to be addressed: https://github.com/dipy/dipy/actions/runs/9605741685/job/26493943578?pr=3266#step:10:10353 (PRs #3268, #3269).
- CVXPY reporting issues: https://github.com/dipy/dipy/actions/runs/9605741677/job/26493933074?pr=3266#step:10:135 We may need to bump the min version in e.g. https://github.com/dipy/dipy/blob/d5e34fbdca1a30829060827f944b5aeb9f145e75/dipy/core/optimize.py#L13 according to https://github.com/cvxpy/cvxpy/issues/2474#issuecomment-2175018262

skoudoro commented 1 week ago

Hi @jhlegarreta,

Thank you for this PR. Can you rebase it?

jhlegarreta commented 1 week ago

Can you rebase it?

Done.

jhlegarreta commented 1 week ago

Run out of ideas to fix the data type size issues :confused:.

skoudoro commented 1 week ago

Hi @jhlegarreta,

I am not sure to understand your approach and what you are trying to achieve in this PR.

Maybe #3274 could help.

I do not think we need cnp.import_array(). Let me know what you think and thank you for your feedback

jhlegarreta commented 1 week ago

I am not sure to understand your approach and what you are trying to achieve in this PR.

c74f656 tried to address the issues as explained in https://github.com/dipy/dipy/pull/3266#issue-2360882282. Those issues are also being reported by the CIs, e.g. https://github.com/dipy/dipy/actions/runs/9691442240/job/26742933573#step:10:11351

E   ImportError: 
numpy.core.multiarray failed to import (auto-generated because you didn't call 'numpy.import_array()' 
after cimporting numpy; use '<void>numpy._import_array' to disable if you are certain you don't need it).

Maybe https://github.com/dipy/dipy/pull/3274 could help.

If I read well, that PR takes (a slight variation of) c5b3f32 and e692764 that exist in this PR.

I do not think we need cnp.import_array(). Let me know what you think and thank you for your feedback

As per your PR, it looks like the cnp.import_array() calls that try to address the mentioned error may be a by-product of the NumPy version used to compile the code, and thus they shall not be required.

skoudoro commented 1 week ago

ok I see now.

The errors in https://github.com/dipy/dipy/pull/3266#issue-2360882282 are due to the DIPY build with the wrong version of Numpy which make sense. Fixing the build fix those errors.

(a slight variation of) https://github.com/dipy/dipy/commit/c5b3f32f16ae1c5c38c42764cb29a0406fec3ff5 and https://github.com/dipy/dipy/commit/e692764bef8c03b354b7b47fb213e1a3b06372a4 that exist in this PR.

the idea was good but both commit are wrong because they do not address correctly the issue. Here, only the build section is important. we need to make sure to always build with Numpy>=2.0.0 and the last cython version allow that.

However dependencies can be lower since the build support the use of lower version of numpy.

So I will close this PR. Thank you so much for working on this and your help in general @jhlegarreta.