- Notifications
You must be signed in to change notification settings - Fork75
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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. |
| 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.
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 thecopy 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 toNone and conditionally issuing a DeprecationWarning when it is set toTrue/False is a viable option? And the deprecation warning would inform the user that the copy-kwarg will be dropped in future release ofquantities.
There was a problem hiding this comment.
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.
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 :) |
Nicely spotted@drammock, hopefully fixed in6d035c6. 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.
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 thearray_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.
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?
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. Ifsomeone wants to take over with a new PR (based on this branch or masterbranch), 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: ***@***.***> |
6d035c6 intopython-quantities:masterUh oh!
There was an error while loading.Please reload this page.
| @@ -9,6 +9,8 @@ | |||
| from .registry import unit_registry | |||
| from .decorators import memoize | |||
| _np_version = tuple(map(int, np.__version__.split('.'))) | |||
There was a problem hiding this comment.
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'There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ofnumpy, 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.