Fix remaining assumptions on 5D dimensions #148
Conversation
A few specifications especially in the HCS domain still rely on the expectation that the underlying image will be 5D. As this constraint has been lifted with v0.3, these changes update various places in the code to dynamically identify the x, y and c axis before performing array manipulation.
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 82.11% 82.40% +0.28%
==========================================
Files 12 12
Lines 1269 1250 -19
==========================================
- Hits 1042 1030 -12
+ Misses 227 220 -7
Continue to review full report at Codecov.
|
|
Trying with: I'm not sure why I'm getting that last error. |
Apologies, that's indeed a single image. https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.3/idr0094A/7751.zarr is the one I had in mind (description updated) |
|
I'm afraid I get the same error: error |
|
Interesting thanks. I hadn't tested with |
|
d61e4d5 allowed me to identify the source of one of the issues i.e. the normalization corrupts the path. c3927c0 disables the keys normalization and the command now fails with with might be an issue at the napari-ome-zarr layer. To be confirmed with @will-moore. |
|
I'm afraid I'm now getting seg-faults every time I run napari, so I'm not currently in a position to look into this any further. I think the |
|
Sounds like another vote for integration tests in |
Seconding this statement, I also feel we need additional test coverage in this repository - which I suspect is also what https://github.com/ome/ome-zarr-py/pull/148/checks?check_run_id=4486647472 is reporting. I had an initial look at extending
Is there a consensus on the best approach? |
|
Last commits allow to fix all HCS integration tests introduced in #153 including the reading of plates with 2D-4D images via I am still failing to get these reading in |
There was a problem hiding this comment.
Big 👍 for the fixes. Have a vague concern about updating the [magic numbers](https://en.wikipedia.org/wiki/Magic_number_(programming) since conceivably we'll need to re-adjust at some point in the future.
| size_y = well_spec.img_shape[3] | ||
| size_x = well_spec.img_shape[4] | ||
| self.axes = well_spec.img_metadata["axes"] | ||
| size_y = well_spec.img_shape[len(self.axes) - 2] |
There was a problem hiding this comment.
How close do you think we are getting to wanting accessors for these?
There was a problem hiding this comment.
Definitely feels like something the Axes object proposed in #124 could handle i.e. self.axes.get_y_index() or similar?
There was a problem hiding this comment.
Thinking more about it, in https://github.com/ome/ome-zarr-py/pull/148/files#diff-b50d9715cc6e4017cfc055fd0ed73ecb5d9158e17f4d58ca5b3ba08b89c46657R581, I used c_index = self.axes.index("c") to avoid relying on magic numbers.
With the current spec, this is also a strategy that could be used here i.e. self.axes.index("y"). However this will also be affected by #124 since the axes representation will become a list of dictionaries rather than a list of strings. @will-moore It looks like we need to decide on a resolution order as this PRs are becoming effectively coupled.
|
The remaining failure is due to the fact that 41c0d8f disables the entire |
|
I imagine ignoring or explicitly excluding some lines from codecov for the moment would be fine. Do we assume that a PlateLabel can't currently exist in the wild? |
|
with this PR without |
| @@ -57,7 +57,7 @@ def __init__( | |||
| self.specs.append(PlateLabels(self)) | |||
| elif Plate.matches(zarr): | |||
| self.specs.append(Plate(self)) | |||
| self.add(zarr, plate_labels=True) | |||
| # self.add(zarr, plate_labels=True) | |||
There was a problem hiding this comment.
maybe a comment indicating why it is commented out?
There was a problem hiding this comment.
Agreed. @joshmoore @will-moore should #65 act as the reference issue for re-enabling/rewriting PlateLabels? In which case the code could directly link to it
There was a problem hiding this comment.
Yes, I think so - Don't fully know the reason for each failure, but they seem closely related
There was a problem hiding this comment.
I'll leave you to open a follow-up PR. Merging for a pre-release
|
|
Fixes #145
A few specifications especially in the HCS domain still rely on the expectation that the underlying image will be 5D. As this constraint has been lifted with v0.3, these changes update various places in the code to dynamically identify the x, y and c axis before performing array manipulation.
Without this PR, both
ome_zarr infoandnaparishould fail to read https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.3/idr0094A/7751.zarr is a suitable example for testing this change (plate with XY dimensions). With this PR included, both commands should work successfully.The last commits also disable
PlateLabelsto fix the HCS display innapari. #177 as well as #65 (comment) might be fixed by this PR