search

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

Deprecate Copy Behavior to support Quantities 16.0 #1554

Closed zm711 closed 1 month ago

zm711 commented 2 months ago

I want to do this in a couple parts. First remove copy. Then ensure tests are passing (currently I've commented out the tests, but my plan is to delete the copy=True tests). Just so people can monitor progress I'll just have this open as a PR.

Fixes #1534 Fixes #1529

zm711 commented 2 months ago

Things I found for us to discus:

1) we can't change the units or dtype during construction since we only get a view of the array. I deleted all tests changing units/dtype 2) time_shift required a different strategy so I implemented it 3) deleted all tests checking for copy vs view

4) I need help with the pickle tests. Reading from pickle just returns a None instead of the appropriate array. Someone who knows pickle better should check this for me. Currently they are commented out.

zm711 commented 2 months ago

IO tests failing due to a CI issue right now.... We can check that later.

So this issue seems to actually be due to failure on python 3.11. I can't find a reason google other than saying the wrong *.so file was probably downloaded. Maybe there will be a fix, but not sure.

@h-mayorquin if you have two minutes sometime today could you look at the CI error for IO tests here. I've tried googling but it just seems like there is a package misalignment. I'm not sure from where. You definitely don't have to try to debug yourself, but if you've seen this before on any of your CIs I would appreciate your insights.

h-mayorquin commented 2 months ago

Try flushing the conda cashes in the CI just to see if the dependency issue goes away.

zm711 commented 2 months ago

So now we have an issue in nix that I have propagated the fix for quantities here.

I just have pickleio that is broken and I don't know why. @samuelgarcia or @apdavison can either of you walk me through the pickling story a bit more.

zm711 commented 1 month ago

@apdavison, if you have the brain power to look over this. We are running into one stumbling block with pickleio not working. Sam and I looked at it and we aren't quite sure why. The nix test failure needs to be fixed with my PR at quantities level.

samuelgarcia commented 1 month ago

@zm711 : I fixes the pickleio tsuff it was on the copy=True was still here in the __reduce__ (used by pickle)

zm711 commented 1 month ago

Perfect thanks for the pickleio. I'll uncomment the core pickle and then we have the nix fix that needs to happen in quantities.

zm711 commented 1 month ago

@apdavison, I convinced Sam to help with the pickle a bit more. So last thing we need for tests to pass is for a release of quantities with https://github.com/python-quantities/python-quantities/pull/242 merged in. Tests have passed there. Then you can do the final review for this!

apdavison commented 1 month ago

https://pypi.org/project/quantities/0.16.1/

h-mayorquin commented 1 month ago

Great!

zm711 commented 1 month ago

can we keep the numpy limit and do that in a second pull request? we have some numpy api issues that I would prefer to do in a separate testing framework. We also can't use our IO tests with 2.0 since 2.0 requires more recent versions to work. So I would need to rewrite our core action for the divide created by 2.0.