Add support for NumPy 2.0#232
Conversation
|
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: Can you comment on why you used |
|
Sure, it's my understanding that I found this answer on stackoverflow to be helpful: |
|
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. |
| return np.asanyarray(data, dtype=dtype).view(cls) | ||
|
|
||
| ret = np.array(data, dtype=dtype, copy=copy).view(cls) | ||
| ret = np.asarray(data, dtype=dtype).view(cls) |
There was a problem hiding this comment.
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.
|
one other NumPy 2 compat fix would be removing the reference to |
|
I'm going to link the Neo CI-update so I can see when this is merged to check our tests again :) |
52788ed to
63a64b2
Compare
63a64b2 to
6d035c6
Compare
|
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 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. |
| return self.__array_prepare__(obj, context) | ||
| else: | ||
| return super().__array_wrap__(obj, context, return_scalar) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@bjodah just wanted to bump my message. Does this seem like it could be the problem with the tests failing?
|
Sounds plausible. I'm on vacation at the moment, and then I'll be moving,
so I won't have much time the upcoming weeks to look further into this. If
someone wants to take over with a new PR (based on this branch or master
branch), then feel free to do so.
…On Wed, Jul 24, 2024, 14:41 Zach McKenzie ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In quantities/quantity.py
<#232 (comment)>
:
> + return self.__array_prepare__(obj, context)
+ else:
+ return super().__array_wrap__(obj, context, return_scalar)
@bjodah <https://github.com/bjodah> just wanted to bump my message. Does
this seem like it could be the problem with the tests failing?
—
Reply to this email directly, view it on GitHub
<#232 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADWUMA3JMNEHGEOFEZ7IVTZN6OF5AVCNFSM6AAAAABJPQSL22VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJWGYZDQNJTGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
|
That's great to hear! And thanks.
…On Wed, Jul 24, 2024, 19:29 Zach McKenzie ***@***.***> wrote:
Thanks @bjodah <https://github.com/bjodah>, I built off of your PR in #235
<#235> and got
it working for both. Enjoy the vacation and moving :)
—
Reply to this email directly, view it on GitHub
<#232 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADWUMGXZTDP4XWWJ4HL72LZN7P7DAVCNFSM6AAAAABJPQSL22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBYGU2DQMZQHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| from .registry import unit_registry | ||
| from .decorators import memoize | ||
|
|
||
| _np_version = tuple(map(int, np.__version__.split('.'))) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😃
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.