Skip to content
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

Fix tests for quantities 0.16.0 #1534

Closed
zm711 opened this issue Aug 27, 2024 · 12 comments · Fixed by #1554
Closed

Fix tests for quantities 0.16.0 #1534

zm711 opened this issue Aug 27, 2024 · 12 comments · Fixed by #1554
Assignees
Labels
Milestone

Comments

@zm711
Copy link
Contributor

zm711 commented Aug 27, 2024

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

@zm711
Copy link
Contributor Author

zm711 commented Aug 27, 2024

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.

@NeuralEnsemble NeuralEnsemble deleted a comment from YeGop0218 Aug 27, 2024
@zm711
Copy link
Contributor Author

zm711 commented Aug 27, 2024

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
Copy link

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 mne-tools/mne-python#12815

@zm711
Copy link
Contributor Author

zm711 commented Aug 28, 2024

@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
Copy link
Member

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

@zm711
Copy link
Contributor Author

zm711 commented Aug 28, 2024

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

@mscheltienne
Copy link

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
Copy link
Contributor Author

zm711 commented Aug 28, 2024

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
Copy link

No problem if you need the release anyway :)

@zm711
Copy link
Contributor Author

zm711 commented Sep 4, 2024

@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
Copy link
Contributor Author

zm711 commented Sep 13, 2024

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
Copy link
Member

Moritz-Alexander-Kern commented Sep 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants