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

feat: remove save logic and use pymmcore-plus writer from MDAWidget #315

Merged
merged 25 commits into from
Mar 6, 2024

Conversation

fdrgsp
Copy link
Contributor

@fdrgsp fdrgsp commented Feb 18, 2024

This PR only removes the saving logic that we had here in napari-micromanager and uses the pymmcore_plus writers implemented in the MDAWidget (pymmcore-plus/pymmcore-widgets#279).

see also #316

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.49%. Comparing base (e321203) to head (ec2a4e4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
- Coverage   79.22%   78.49%   -0.73%     
==========================================
  Files          15       13       -2     
  Lines         693      623      -70     
==========================================
- Hits          549      489      -60     
+ Misses        144      134      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fdrgsp fdrgsp changed the title feat: remove save logic and use pymmcore-plus writer from MDAWidget [WIP]feat: remove save logic and use pymmcore-plus writer from MDAWidget Mar 2, 2024
@fdrgsp fdrgsp changed the title [WIP]feat: remove save logic and use pymmcore-plus writer from MDAWidget feat: remove save logic and use pymmcore-plus writer from MDAWidget Mar 3, 2024
@fdrgsp fdrgsp changed the title feat: remove save logic and use pymmcore-plus writer from MDAWidget [WIP] feat: remove save logic and use pymmcore-plus writer from MDAWidget Mar 4, 2024
@fdrgsp fdrgsp changed the title [WIP] feat: remove save logic and use pymmcore-plus writer from MDAWidget feat: remove save logic and use pymmcore-plus writer from MDAWidget Mar 4, 2024
@@ -8,10 +8,11 @@

import napari
import zarr
from pymmcore_plus.mda.handlers._util import get_full_sequence_axes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never import private stuff from other packages, even if it's your package. I know it's annoying. but it just always comes back to bite you. I'll update this one

@tlambert03
Copy link
Member

tlambert03 commented Mar 5, 2024

ok, i've had a look over and it looks good!

I did add back the test_saving_mda test (we need to have at least one test to ensure we're saving things properly, even if it's not handled here). But in doing so, I hit one error in the expected data shape. You should see the error if you run:

pytest -k 'test_saving_mda and no_splitC'

        assert dest.exists()
>       assert data_shape == expected_shape
E       assert [3, 7, 2, 500, 512] == [3, 2, 7, 500, 512]
E
E         At index 1 diff: 7 != 2
E         Use -v to get more diff

any thoughts as to why the data is transposed?

@tlambert03
Copy link
Member

figured it out. the mda_widget appears to be changing the axis_order of our requested mda. looking into it

@tlambert03
Copy link
Member

fixes by pymmcore-plus/pymmcore-widgets#286

@tlambert03
Copy link
Member

looks like we're now hitting this bug: pymmcore-plus/pymmcore-plus#334

so there really is something to that one. Will see if I can recreate in pymmcore-plus and fix

@tlambert03
Copy link
Member

bug is fixed upstream, all passing now, will merge

@tlambert03 tlambert03 merged commit c9f5d30 into pymmcore-plus:main Mar 6, 2024
18 checks passed
@fdrgsp fdrgsp deleted the use_pymmcore_writers branch May 27, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants