-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add image IO loading code from cellfinder & create image IO submodule within IO #73
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 90.38% 89.76% -0.62%
==========================================
Files 35 35
Lines 1393 1427 +34
==========================================
+ Hits 1259 1281 +22
- Misses 134 146 +12 ☔ View full report in Codecov by Sentry. |
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.
Thanks for this @adamltyson !
It largely looks good to me, but I noticed a potential problem around being too strict with tiff axis metadata (similar to one we had previously). I suggest applying the same approach as before (I can do it and add to this PR if you like... let me know.)
) | ||
|
||
axes = tiff.series[0].axes.lower() | ||
if set(axes) != {"x", "y", "z"} or axes[0] != "z": |
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.
This is probably too strict? We missed this on initial review, but caught this on a later review of related functionality. I expect we want to be more permissive here based on previous discussions around how clean we expect TIFF metadata to be. At the very least, we should be consistent between read_z_stack
and get_size_image_from_file_paths
functions?
I suggest doing the equivalent of ed40e38
(#67) for read_z_stack
.
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.
The code added here is just copied and pasted from cellfinder, so should these changes go into a different PR? Or did I miss something?
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.
You didn't miss anything.
Sure, maybe it's better to put it in a different PR.
We should get that PR merged before releasing though, otherwise users that don't have xyz
(in any order) in their tiff metadata will experience a change in behaviour.
This PR does a few things:
brainglobe_utils.image_io.load
cellfinder#405)image_io
intoIO
? #69)Once merged, this should be released as 0.5.0 (to match the pinning in other packages)
Related PRs: