-
Notifications
You must be signed in to change notification settings - Fork 4
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
Depend on iohub
#331
Depend on iohub
#331
Conversation
Preview page for your plugin is ready here: |
iohub requires python 3.9+. Should this be a separate issue or we just bump it here? |
Co-authored-by: Ziwen Liu <67518483+ziw-liu@users.noreply.github.com>
Thanks! I think we can bump it here. |
Codecov Report
@@ Coverage Diff @@
## main #331 +/- ##
========================================
- Coverage 4.23% 2.06% -2.17%
========================================
Files 23 21 -2
Lines 5150 4888 -262
========================================
- Hits 218 101 -117
+ Misses 4932 4787 -145
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@ziw-liu thanks for your help here. I'm calling this "ready for review" because I need to keep driving this forward. I tested on hummingbird this morning and I'm planning to continue testing tomorrow. Can I request complete review and that you spend some time testing on hummingbird and/or automaton? When this merges I think it's ready to be released as |
@ziw-liu following our discussion I've now removed |
Co-authored-by: Ziwen Liu <67518483+ziw-liu@users.noreply.github.com>
@talonchandler Should we do anything to the codecov workflow? Our test coverage is not ideal, but I don't think it has to fail for this PR. It's not blocking the merge though so we can also deal with it later. |
I agree that test coverage shouldn't block this merge. We can discuss more in the meeting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Last minute request: can we remove these lines? Lines 56 to 60 in 6742be9
|
This PR makes
recOrder
depend on the newiohub
readers/writers, and removes the dependency onWaveorderReader
andWaveorderWriter
.The major changes are:
recOrder
acquisitions and metadata now save to ome.zarr v0.4recorder view
now usesiohub
and can open ome.zarr v0.4 (in addition to v0.1 and MM datasets).recorder view
is also no longer "officially supported" (i.e. it does not appear on README figure) since we expect its behavior to change significantly and it should not block our 0.3 and 0.4 releases.recorder/examples
scripts write toiohub
recorder info
andrecorder convert
are removed - improved alternatives live iniohub
python 3.8
support and adds3.11