-
Notifications
You must be signed in to change notification settings - Fork 54
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
Read multi-resolution pyramids in Well class (closes #208) #209
Conversation
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 83.86% 83.90% +0.03%
==========================================
Files 12 12
Lines 1376 1379 +3
==========================================
+ Hits 1154 1157 +3
Misses 222 222
Continue to review full report at Codecov.
|
This is working fine, (once I'd worked out how to allow Well viewing in Opening a Well e.g.
I initially see lower resolution tiles being loaded..
On zooming in, I see full resolution tiles being loaded..
|
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.
Code looks good too. 👍
cc @sbesson
ome_zarr/reader.py
Outdated
column_count = math.ceil(math.sqrt(field_count)) | ||
row_count = math.ceil(field_count / column_count) | ||
self.row_count = math.ceil(math.sqrt(field_count)) | ||
self.column_count = math.ceil(field_count / self.row_count) |
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.
Was there a reason to switch the order of column and row calculation here? Does it make any difference?
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 was not supposed to slip into the PR, indeed.
I just took it out in c965afe.
FYI, The reason was that for a non-square well I was getting an 8x9 shape instead of the 9x8 shape I needed. But I'm not expecting the Well
class to support non-square geometry in a reliable way, in its current implementation. If needed, I would rather make the required shape more explicit (e.g. by passing an optional grid_shape
argument to Well
).
Would you suggest we release this as |
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.
Great, thanks. LGTM
Re-reading the details of the proposed changes, I convinced myself that the |
Does 35cdce0 solve this issue? EDIT: I only got it now, the point was about 0.5.1 vs 0.6.0, my bad. The question remains: would you rather have those two functions as |
I don't have a strong opinion on the last commit, but slight preference for reducing the diff for this PR, so 👍 - and I re-tested the change and everything is still working. |
Thanks all this is now released as https://pypi.org/project/ome-zarr/0.5.1/ |
Fixes #208, by replicating a structure similar to the
Plate
class into theWell
class (i.e. reading multi-resolution pyramids).Logs for
napari -vvv --plugin napari-ome-zarr xxx.zarr/B/03/
(also including a bit of zooming in/out, to trigger more loading) show that different levels are actually loaded:Click to expand!
[this example is for an image where the highest-resolution level has Channel-ZYX shape
(3, 1, 19440, 20480)
]