Deprecate Copy Behavior to support Quantities 16.0 #1554
Conversation
|
Things I found for us to discus:
|
|
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 @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. |
|
Try flushing the conda cashes in the CI just to see if the dependency issue goes away. |
|
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. |
|
@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. |
| (the original :class:`IrregularlySampledSignal` is not modified). | ||
| """ | ||
| new_sig = deepcopy(self) | ||
| # As of numpy 2.0/quantities 0.16 we need to copy the array itself |
| units = pq.quantity.validate_dimensionality(units) | ||
|
|
||
| new_st = self.__class__(signal, t_start=t_start, t_stop=t_stop, waveforms=waveforms, units=units) | ||
| signal = deepcopy(signal) |
|
|
||
| class TestAnalogSignalFunctions(unittest.TestCase): | ||
|
|
||
| ## someone with more pickle knowledge needs to work on this |
|
|
||
|
|
||
| class TestAnalogSignalFunctions(unittest.TestCase): | ||
| # pickle help needed |
|
@zm711 : I fixes the pickleio tsuff it was on the copy=True was still here in the |
|
Perfect thanks for the pickleio. I'll uncomment the core pickle and then we have the nix fix that needs to happen in quantities. |
|
@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 python-quantities/python-quantities#242 merged in. Tests have passed there. Then you can do the final review for this! |
|
Great! |
apdavison
left a comment
apdavison
left a comment
There was a problem hiding this comment.
The tests are now passing. Other than the fixes to version numbers in pyproject.toml, this looks fine.
|
|
||
| dependencies = [ | ||
| "packaging", | ||
| "numpy>=1.22.4,<2.0.0", |
There was a problem hiding this comment.
The dependency versions need to be fixed ("numpy>=1.22.4", "quantities>=0.16.1").
There was a problem hiding this comment.
quantities yes. But do we want to do the numpy in 2 PRs? This one to get quantities ready and then if numpy isn't ready we mark the limit and fix the separate numpy issues? Let's free it and see what happens :)
There was a problem hiding this comment.
it was more the lower limit I was talking about, which was going back to 1.19 when I reviewed this.
apdavison
changed the title
|
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. |
4 participants
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