Extract S dim from OME data #483
Conversation
| @@ -189,14 +189,19 @@ def _expand_dims_to_match_ome( | |||
| # need to correct channel count if this is a RGB image | |||
| n_samples = ome.images[scene_index].pixels.channels[0].samples_per_pixel | |||
| for d in dims: | |||
| # SizeC can represent RGB (Samples) data rather | |||
| # than channel data, whether or not this is the case depends | |||
| # on what the SamplesPerPixel are for the channel | |||
There was a problem hiding this comment.
is there a scenario where you can have N channels and M samples and SizeC is set to N*M? Do you have to do a bit of math to find the true N value? Or is that automatically handled here. I remember that coming up a long time ago as a possibility.
There was a problem hiding this comment.
The channel dimension shape is determined by line 196 rather than line 200 so instead of using SizeC it uses the length of the channels list to assign the size of the channel dimension, likely already for this reason so I think we are covered there
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
==========================================
+ Coverage 94.18% 94.20% +0.01%
==========================================
Files 48 48
Lines 4265 4279 +14
==========================================
+ Hits 4017 4031 +14
Misses 248 248
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| """ | ||
| dims_from_ome = metadata_utils.get_dims_from_ome(ome, scene_index) | ||
|
|
||
| dims_from_tiff_axes = list(tiff.series[scene_index].axes) |
There was a problem hiding this comment.
implicit assumption that the dim names coming from tiffffile are compatible with our TCZYXS?
There was a problem hiding this comment.
Ah yes, I'll comment on this. I'd assumed we were already dependent on that assumption based on these lines, but perhaps this comment is just refering to non-TCZYXS dimensions.
| @@ -65,7 +65,7 @@ def _general_data_array_constructor( | |||
| ) -> xr.DataArray: | |||
|
|
|||
| # Unpack dims and coords from OME | |||
| _, coords = metadata_utils.get_dims_and_coords_from_ome( | |||
| coords = metadata_utils.get_coords_from_ome( | |||
There was a problem hiding this comment.
Nice! Good call separating these out 🙂
Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
@AetherUnbound There are tests that cover this in terms of testing the code path and logic, however this issue does refer specifically to an OME TIFF where the dimensions were ordered SYX which is different than the test files the package currently has. I wasn't sure if this was enough of a case to add a new test file or what the criterion were for adding new test files. If it seems like it would be worth it lmk & I'll happily add it! |
|
Ah okay, no worries! I figured since we were adding a new condition that perhaps some test might need to be changed, but if that's dependent on a new file being added then I'm of less help there 😅 |
Description
#457 describes an error where a RGB OME TIFF fails to open. This error occurs because the dimensions of the file are SYX and currently the
OmeTiffReaderonly supports the Samples dimension being at the end of the dimension order via this line.How does this PR address the issue?
The
OmeTiffReaderwas already aware of theSizeCoccasionally being representative of RGB data whenSamplesPerPixelwas present elsewhere in the code. However, it just incorrectly assumed the Samples dimension is the last dimension of the image data.Testing
Tested locally with file from issue as well as some random OME TIFFs from AICS