Skip to content

Add support for NumPy 2.0 #232

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

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

bjodah
Copy link
Contributor

@bjodah bjodah commented Jun 18, 2024

Seems like NumPy 2.0 broke some behavior which quantities relied on. This PR allows the test suite to pass for me locally. I have not addressed any deprecation warnings that are new in numpy 2.0. I'm thinking that we can fix those in a subsequent PR.

Let me know what you think.

@apdavison
Copy link
Contributor

Hi @bjodah - many thanks for this. To help understand why these changes were needed, I created #233. It seems that almost all the errors are of the form:

ValueError: Unable to avoid copy while creating an array as requested.
If using `np.array(obj, copy=False)` replace it with `np.asarray(obj)` to allow a copy when needed (no behavior change in NumPy 1.x).
For more details, see https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword.

Can you comment on why you used np.asanyarray in some places?

@bjodah
Copy link
Contributor Author

bjodah commented Jun 18, 2024

Sure, it's my understanding that asanyarray corresponds to subok=True, and the use of asarray and asanyarray will avoid copying if possible, but unlike the copy=False flag, they will not raise an exception when a copy is needed.

I found this answer on stackoverflow to be helpful:
https://stackoverflow.com/a/52103839/790973

@apdavison
Copy link
Contributor

I will leave this PR open until the end of this week in case anyone else has comments, but if everything still looks good by then, I'll merge this and make a new release.


ret = np.array(data, dtype=dtype, copy=copy).view(cls)
ret = np.asarray(data, dtype=dtype).view(cls)
Copy link
Contributor Author

@bjodah bjodah Jun 18, 2024

Choose a reason for hiding this comment

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

After these changes we are no longer using the copy keyword argument passed into __new__. I contemplated dropping it, but I fear that the change would be too disruptive for backwards compatibility. Perhaps changing its default to None and conditionally issuing a DeprecationWarning when it is set to True/False is a viable option? And the deprecation warning would inform the user that the copy-kwarg will be dropped in future release of quantities.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea.

@drammock
Copy link
Contributor

one other NumPy 2 compat fix would be removing the reference to np.core on this line:

p_dict[np.core.umath.clip] = _d_clip

@zm711
Copy link
Contributor

zm711 commented Jun 18, 2024

I'm going to link the Neo CI-update so I can see when this is merged to check our tests again :)

NeuralEnsemble/python-neo#1490

@bjodah bjodah force-pushed the support-for-numpy-2 branch from 63a64b2 to 6d035c6 Compare June 19, 2024 09:18
@bjodah
Copy link
Contributor Author

bjodah commented Jun 19, 2024

Nicely spotted @drammock, hopefully fixed in 6d035c6. Thanks.

I went ahead and fixed all warnings reported during the test run (when using numpy-2.0). We still carry __array_prepare__ which I think is no longer used externally by numpy, but we do call this method from __array_wrap__, so I did not refactor that part.

I seem to have messed up something else for numpy<2 in last commit. Will need to take a closer look a bit later if no one beats me to it.

@drammock
Copy link
Contributor

Nicely spotted @drammock, hopefully fixed in 6d035c6. Thanks.

don't thank me, thank our CIs :)

Comment on lines +296 to +298
return self.__array_prepare__(obj, context)
else:
return super().__array_wrap__(obj, context, return_scalar)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this. The previous array wrap would only do something if the object was not a Quantity if the object was already a quantity it would just return the object without any changes. Here you force the array_wrap even in the case of the object being a Quantity. It seems like for the case of some failing tests the objects are losing their dimensionality and this seems like it could be a place where a Quantity should be returned, no?

I'm not super familiar with this deep level of array wrapping so just thought maybe a discussion of this could help point us to the actual test failure issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjodah just wanted to bump my message. Does this seem like it could be the problem with the tests failing?

@bjodah
Copy link
Contributor Author

bjodah commented Jul 24, 2024 via email

@zm711
Copy link
Contributor

zm711 commented Jul 24, 2024

Thanks @bjodah, I built off of your PR in #235 and got it working for both. Enjoy the vacation and moving :)

@bjodah
Copy link
Contributor Author

bjodah commented Jul 24, 2024 via email

@apdavison apdavison merged commit 6d035c6 into python-quantities:master Aug 27, 2024
20 of 44 checks passed
@@ -9,6 +9,8 @@
from .registry import unit_registry
from .decorators import memoize

_np_version = tuple(map(int, np.__version__.split('.')))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now breaking the tests in MNE:

     _np_version = tuple(map(int, np.__version__.split('.')))
E   ValueError: invalid literal for int() with base 10: 'dev0+git20240824'

x-ref: mne-tools/mne-python#12815

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you all testing against a development version of NumPy? I don't think @bjodah or I thought about that in these PRs for parsing the numpy version. I guess a version parse instead would be better if you want to defend against dev version testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

MNE is testing against stable and the development version of numpy, all my projects are also tested against both, and I'm probably not the only one ;)
Quick fix in #238 but maybe you want something a bit more elaborate 😃

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.

5 participants