NeuralEnsemble / python-neo

Neo is a package for representing electrophysiology data in Python, together with support for reading a wide range of neurophysiology file formats
http://neo.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
325 stars 248 forks source link

Fix tests for quantities 0.16.0 #1534

Closed zm711 closed 1 month ago

zm711 commented 3 months ago

core-tests failing with new version of quantities. We will need to fix.

zm711 commented 3 months ago

So the new behavior of quantites for numpy 2.0 is to only provide the option to have views of the data. This is causing the test failures seen here. We need to decide whether moving forward do we want to support the copy functionality at the Neo level now that we don't have this option at the quantities level or we remove those tests. Since some of this is related to the Neo.io/Neo.core level I think @samuelgarcia and @apdavison should really comment on the vision for this moving foward with NumPy 2.0 before I start a big PR for this.

zm711 commented 3 months ago

I suggest we limit Neo to quantities< 0.16.0 and then we can have a PR that actually begins fixing for the new quantities version. Then after fixing for quantities we can then fix for numpy 2.0

@alejoe91, I'll tag you as well for this :)

mscheltienne commented 3 months ago

I suggest we limit Neo to quantities< 0.16.0

If you could do this and cut a release, it would be great at the MNE-Python end 🙏. I got stuck trying to get our CIs green temporary in https://github.com/mne-tools/mne-python/pull/12815

zm711 commented 3 months ago

@apdavison, @alejoe91, @samuelgarcia do we want to do one more point release with the quantities limit (now merged) and adding a numpy < 2.0 limit (will need to add) for users that want a working pypi version while we update the code on main? I'm fine doing it all as long as we are all on board (we just have the biocam/plexon updates currently). Then maybe we can discuss how we want to fix things in a meeting before starting a PR for quantities 0.16 and numpy 2.

apdavison commented 3 months ago

We had a request for this from MNE-Python, so if you have time please go ahead with a point release.

zm711 commented 3 months ago

Sounds good I'll work on this today/tomorrow so we will have the point release by end of the week.

mscheltienne commented 3 months ago

To be honest, I don't think the point release with a pin <2 on numpy will be useful to us, so I think you can skip it and work straight on the numpy 2 compatibility :)

zm711 commented 3 months ago

I think we likely need to have this point release more due to the quantities change. The loss of copy actually affects our library for io and core level. It doesn't affect our rawio level, so not sure how you're using the library, but I think we need one release that explicitly pins things while we fix the copy behavior for any of our io users. :) (the numpy isn't super important for us at a developer-level but it more for users of our IO level who we don't want to have a busted install with Neo) But definitely ping us if you see something!

mscheltienne commented 3 months ago

No problem if you need the release anyway :)

zm711 commented 2 months ago

@Moritz-Alexander-Kern,

when you have a moment do you want to comment on this since I know Elephant uses Neo objects. How do you currently interact with the copy argument of neo objects. Would you have an opinion as we move forward with the transition to quantities 0.16.0 and numpy 2.0.

zm711 commented 2 months ago

So we have decided to drop support for copy so that we can be completely NumPy 2.0 compatible. The idea being that if NumPy arrays no longer allow copy then we won't support the copy argument either. For the next release we will keep the copy argument but switch it to None. If True/False is given we will raise an error. This will allow users to switch. @Moritz-Alexander-Kern we checked elephant and you are using Neo copy and Quantities copy. Since quantities copy has already been dropped we assume you'll need to do a refactor to account for this so we hope that by pushing these Neo changes soon you'll be able to do a refactor of both together.

Moritz-Alexander-Kern commented 2 months ago

Hey @zm711 , Indeed, that is a good opportunity to refactor this. Thanks.