Support for NumPy 2.0 in Neo Core #1490
Conversation
|
Looks like we have some work to do :) |
|
note that quantities 0.16.0, with support for NumPy 2.0, has just been released |
|
I updated the branch so we should see the neo problems vs the quantities in the testing! |
|
I think @JuliaSprenger saw this error before right? Actually this is an example of an error related to the fact we no longer have the copy behavior. this test was an assertNotEqual, but because the data is now a copy we edited in place and so the array is the same when it shouldn't be. |
|
Let's squash this one when it is ready. A lot of this is me tweaking the action and then messing up on one version. So a lot of these are trash commits. |
|
Okay current core failures can be reproduced with the following: >>> import quantities as pq
>>> import numpy as np
>>> times = np.arange(10, dtype=np.float32)
>>> times.dtype
'float32'
>>> times= times * pq.ms
>>> times.dtype
'float64'and for the other failing test >>> import quantities as pq
>>> import numpy as np
>>> times = [1,2,3] * pq.s
>>> np.concatenate((times, times))
array([1,2,3,1,2,3]) * dimensionlessI need to read more about numpy 2.0 changes to figure out these parts of the tests that have changed. Any ideas @apdavison and @samuelgarcia ? The first issue seems like it could be numpy but the second issue seems like quantities needs to hand np.concatenate better no? |
|
Actually it was a change in default numpy behavior which requires an extra as type in tests. My bad. |
|
Once #1585 is in main we can merge main into this which fixes the RTD error. Then this ready. Note this is only for the core of Neo. I want to do the RawIO/IO in a separate PR. |
| @@ -975,7 +975,7 @@ def _merge_array_annotations(self, others, sorting=None): | |||
|
|
|||
| omitted_keys_other = [ | |||
| key | |||
| for key in np.unique([key for other in others for key in other.array_annotations]) | |||
| for key in set([key for other in others for key in other.array_annotations]) | |||
There was a problem hiding this comment.
only actual change to code base. numpy 2.0 returns this as a np.str_ thing rather than as the actual string. Using set instead of np.unique returns the string both >2.0 and < 2.0 for NumPy. Maybe a slight performance hit, but probably not too bad since the annotations should be small either way and this is in a list and not array.
zm711
changed the title
|
Also a reminder please squash before merge and let's at minimum wait until after the point release :) |
| @@ -1147,7 +1155,7 @@ def test_merge_multiple(self): | |||
| expected *= time_unit | |||
| sorting = np.argsort(expected) | |||
| expected = expected[sorting] | |||
| np.testing.assert_array_equal(result.times, expected) | |||
| np.testing.assert_array_equal(result.times.magnitude, expected.magnitude) | |||
There was a problem hiding this comment.
This change is necessary because as of numpy 2.0 because numpy assert_array_equal actually checks that the quantities are the same which leads to a units issue. If someone else understands this better please post here.
There was a problem hiding this comment.
This is annoying @apdavison any comments ?
|
RTD is not building the correct version of neo. I don't know why. Maybe I just need to give it a little time (I had hoped switching to 3.12 would flush any cache, but it still had the wrong Neo and it had other errors). I'll just try to rerun RTD build this weekend and see if time fixes it. |
|
Sam and I looked over the RTD failure and we aren't sure why it is happening. I think it should fix itself once this is merged into main since the problem showing up is fixed in main. |
| times = np.arange(10, dtype="f4") * pq.ms | ||
| # this step is required for NumPy 2.0 which now casts to float64 in the case either value/array | ||
| # is float64 even if not necessary | ||
| # https://numpy.org/devdocs/numpy_2_0_migration_guide.html | ||
| times = times.astype(dtype="f4") |
There was a problem hiding this comment.
Based on discussion with Sam my comment may not have been clear enough. The problem is that
1* pq.ms is at baseline at 'float64' and as of numpy 2.0+ everything is up-cast to the bigger dtype even if not necessarily. So in old versions of our test times would remain a float32, but with new versions of numpy make it into a 'float64'. I did the astype to keep our original idea explict, but we could think of other rewrites of this test to maintain the dtype test.
|
@apdavison this is ready for review, the RTD is broken with some cache issue, but I fixed the problem on main (and merging main into this branch didn't fix it). so our options are you review and merge ignoring the RTD problem or I open a new PR built off of the current main and make the exact same fixes to make sure all tests pass. What's your preference? |
following a policy of NEP 29 + 1 year.