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

Replace np.sctypes for numpy 2.0 compat #1250

Merged
merged 51 commits into from
Nov 2, 2023
Merged

Conversation

mscheltienne
Copy link
Contributor

@mscheltienne mscheltienne commented Aug 28, 2023

With nightly numpy release, the following attribute error is raised:

AttributeError: `np.sctypes` was removed in the NumPy 2.0 release. . Did you mean: 'dtypes'?

corresponding to this bullet point in the release notes:

np.issctype, np.maximum_sctype, np.obj2sctype, np.sctype2char, np.sctypeDict, np.sctypes, np.issubsctype were all removed from the main namespace without replacement, as they where niche members.

This PR replaces:

  • np.sctypes calls with np.core.sctypes
  • arr.newbyteorder with arr.view(arr.dtype.newbyteorder())
  • some implicit casts when multiplying by python int to explicit casts then multiply
  • np.can_cast usage

@mscheltienne mscheltienne changed the title Replace np.sctypes with np.core.sctypes Replace np.sctypes with np.core.sctypes for numpy 2.0 compat Aug 28, 2023
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4588058) 92.22% compared to head (a71eebf) 92.25%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1250      +/-   ##
==========================================
+ Coverage   92.22%   92.25%   +0.02%     
==========================================
  Files          99       99              
  Lines       12454    12466      +12     
  Branches     2559     2564       +5     
==========================================
+ Hits        11486    11500      +14     
+ Misses        644      643       -1     
+ Partials      324      323       -1     
Files Coverage Δ
nibabel/conftest.py 100.00% <100.00%> (ø)
nibabel/ecat.py 88.21% <100.00%> (ø)
nibabel/freesurfer/io.py 94.24% <100.00%> (ø)
nibabel/quaternions.py 99.02% <100.00%> (+<0.01%) ⬆️
nibabel/spatialimages.py 94.14% <100.00%> (+0.02%) ⬆️
nibabel/streamlines/trk.py 93.98% <100.00%> (ø)
nibabel/nifti1.py 92.79% <88.88%> (+0.03%) ⬆️
nibabel/casting.py 87.23% <77.77%> (+0.85%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member

Thank you! I'm a little bit dreading cleaning up the tests for numpy 2 (#1242), but happy to take preemptive fixes that don't break existing numpy.

Would you care to add yourself to the Zenodo and author list?

@effigies
Copy link
Member

Actually, would you mind running pipx run blue nibabel to sort out the styling?

@larsoner
Copy link
Contributor

I suspect this is just a workaround since the release notes say:

... np.sctypes, np.issubsctype were all removed from the main namespace without replacement, as they where niche members.

So really it seems like the code should be updated not to use sctypes at all :(

@mscheltienne
Copy link
Contributor Author

I don't read this as a deprecation of sctypes entirely, rather as a removal of the "shortcut" np.sctypes, since they explicitly mention namespace.

@effigies
Copy link
Member

I asked on: numpy/numpy#24376

Not sure how likely they are to see a question on a closed PR, so might ask again in a couple days if we don't get a response.

@mscheltienne
Copy link
Contributor Author

Perfect, at least that will be a clear answer :)

@larsoner
Copy link
Contributor

I don't read this as a deprecation of sctypes entirely, rather as a removal of the "shortcut" np.sctypes, since they explicitly mention namespace.

Looks like np.core should not be used

numpy/numpy#24376 (comment)

@mscheltienne
Copy link
Contributor Author

Well that was quick. I don't have time today, but I can have a look tomorrow to replace sctypes with something else.

Comment on lines 26 to 34
# np.sctypes is deprecated in numpy 2.0 and np.core.sctypes should not be used instead.
sctypes = {
"int": [np.int8, np.int16, np.int32, np.int64],
"uint": [np.uint8, np.uint16, np.uint32, np.uint64],
"float": [np.float16, np.float32, np.float64, np.float128],
"complex": [np.complex64, np.complex128, np.complex256],
"others": [bool, object, bytes, str, np.void],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely convince this is enough to keep a working status on all platforms. It looks to me like np.sctypes is defined per platform, and if this is correct, we might need to add some logic to define the mapping between strings and scalar types per platform as well.

@mscheltienne mscheltienne changed the title Replace np.sctypes with np.core.sctypes for numpy 2.0 compat Replace np.sctypes for numpy 2.0 compat Aug 29, 2023
Comment on lines 27 to 33
sctypes = {
'int': [np.int8, np.int16, np.int32, np.int64],
'uint': [np.uint8, np.uint16, np.uint32, np.uint64],
'float': [np.float16, np.float32, np.float64, np.longdouble],
'complex': [np.complex64, np.complex128, np.complex256],
'others': [bool, object, bytes, str, np.void],
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm out of idea to correctly build this mapping dict on all platforms. I never worked with longlong, longdouble, clongdouble and I am not familiar with the issues that are attached to them. A couple of ideas:

  • Don't use the size aliases and let numpy figure that out (6c30a84). It should work, but compared to that commit the list(set(...)) should be sorted by precision. One point eluded me however: np.byte correctly points to np.int8, np.ubyte to np.uint8, and so on.. except for np.longlong and np.ulonglong which points to themselves. np.ulonglong == np.uint64 is False and I guess that would cause issues.
  • create the mapping based on attributes, e.g. hasattr(np, "float128").

Any good idea?

@effigies
Copy link
Member

Hi, thanks for this, and I think this will work. I need to find a little time to think clearly about this problem. Just to let you know that I haven't forgotten about this, just a little slow to get back to it.

@larsoner
Copy link
Contributor

Looks like this branch is getting out of date, trying to use it in MNE I get:

/opt/hostedtoolcache/Python/3.11.5/x64/lib/python3.11/site-packages/nibabel/quaternions.py:32: in <module>
    MAX_FLOAT = np.maximum_sctype(float)
/opt/hostedtoolcache/Python/3.11.5/x64/lib/python3.11/site-packages/numpy/__init__.py:383: in __getattr__
    raise AttributeError(
E   AttributeError: `np.maximum_sctype` was removed in the NumPy 2.0 release.

nibabel/nifti1.py Outdated Show resolved Hide resolved
@larsoner
Copy link
Contributor

Green! (Other than some coverage misses that I think probably exist in master as well?)

@effigies feel free to review/merge if you're happy, only ended up at +170 −97

nibabel/casting.py Outdated Show resolved Hide resolved
nibabel/casting.py Outdated Show resolved Hide resolved
nibabel/conftest.py Outdated Show resolved Hide resolved
nibabel/quaternions.py Outdated Show resolved Hide resolved
Comment on lines 157 to 165
# TODO: Should this actually still overflow? Does it matter?
if FP_OVERFLOW_WARN:
ctx = pytest.raises(OverflowError)
else:
ctx = nullcontext()
with ctx:
as_int(val)
with pytest.raises(OverflowError):
with ctx:
as_int(-val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with numpy 2.0? This seems like it should still fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not overflow. Agreed it's weird that it does not

nibabel/tests/test_volumeutils.py Outdated Show resolved Hide resolved
nibabel/tests/test_volumeutils.py Outdated Show resolved Hide resolved
Comment on lines 742 to 745
# This is the normal rule - no upcast from Python scalar
# (on NumPy 2.0 it *will* upcast from a np.float64 scalar!)
assert (f32_arr * 1.).dtype == np.float32
assert (f32_arr + 1.).dtype == np.float32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here seems to be contrasting to numpy casting rules, not numpy + Python scalar casting rules. I would just branch on numpy version about the expected dtype for using f64(...).

@effigies
Copy link
Member

effigies commented Nov 2, 2023

Okay, I'm starting to understand what's going on here. Numpy previously had broken behavior around longdoubles and ints. If you install Python 3.6 and numpy 1.11, you get things like:

In [1]: import numpy as np

In [2]: val = np.longdouble(2**1024) * 2
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
<ipython-input-2-4bc60941a286> in <module>
----> 1 val = np.longdouble(2**1024) * 2

OverflowError: int too large to convert to float

In [6]: val = np.longdouble(2**1023)

In [7]: val
Out[7]: 8.9884656743115795386e+307

In [8]: val * 2
Out[8]: 1.7976931348623159077e+308

In [9]: val * 4
Out[9]: 3.5953862697246318155e+308

In [10]: int(val * 4)
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
<ipython-input-10-af59c627c981> in <module>
----> 1 int(val * 4)

OverflowError: cannot convert float infinity to integer

However, if you use Python 3.8 and numpy 1.20 (nibabel current minimum requirements):

In [1]: import numpy as np

In [2]: nexp64 = 1024

In [3]: val = np.longdouble(2**1024) * 2

In [4]: val
Out[4]: 3.5953862697246318155e+308

In [5]: int(val)
Out[5]: 359538626972463181545861038157804946723595395788461314546860162315465351611001926265416954644815072042240227759742786715317579537628833244985694861278948248755535786849730970552604439202492188238906165904170011537676301364684925762947826221081654474326701021369172596479894491876959432609670712659248448274432

I think at this point, probably the correct thing is just to get rid of int_to_float and as_int. But that's not needed for this PR.

@effigies effigies merged commit dabfc46 into nipy:master Nov 2, 2023
51 of 52 checks passed
@mscheltienne mscheltienne deleted the np.sctypes branch November 2, 2023 14:26
MarDiehl added a commit to MarDiehl/DAMASK that referenced this pull request Jun 19, 2024
MarDiehl added a commit to MarDiehl/DAMASK that referenced this pull request Jun 19, 2024
MarDiehl added a commit to MarDiehl/DAMASK that referenced this pull request Jun 19, 2024
MarDiehl added a commit to MarDiehl/DAMASK that referenced this pull request Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants