bugfix: removing 2.11.0 threshold

[BrianWhitneyAI]

How did this come about?

This pull request was opened to resolve #361. At the time of this bug, several tests were failing with:

RuntimeError: `mp4` can not handle the given uri.

The quick-fix solution was to pin the dependency to "imageio[ffmpeg]>=2.9.0,<2.11.0". Returning to these tests, I was unable to replicate these errors, testing with imageio (2.10.5, 2.11.0,2.12.0, 2.26.0(current)). This is likely an indication that this issue was resolved internally by the imageio team.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (3175843) 94.08% compared to head (b9e45fb) 94.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #470      +/-   ##
==========================================
+ Coverage   94.08%   94.16%   +0.07%     
==========================================
  Files          48       48              
  Lines        4162     4197      +35     
==========================================
+ Hits         3916     3952      +36     
+ Misses        246      245       -1     

Impacted Files	Coverage Δ
...tests/readers/extra_readers/test_default_reader.py	100.00% <ø> (ø)
aicsimageio/readers/default_reader.py	97.69% <100.00%> (+0.76%)	⬆️

... and 4 files 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.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[evamaxfield]

Glad that the error has gone away but can we fix the single failing test before merging

[SeanLeRoy]

Glad that the error has gone away but can we fix the single failing test before merging

This is captured elsewhere and is seemingly also present on main. Working on a fix for that on another branch, we'll keep this open until then.

[SeanLeRoy]

Looks good, I'd just double check all the errors go away when combined with the fixed main branch

[toloudis]

maybe merge main into here and see if tests go green?

[toloudis]

hm, looks like there are test failures on this one...

[BrianWhitneyAI]

hm, looks like there are test failures on this one...

yep. I am looking into this right now. It is definitely from changing to imageio >= 2.11.0 which causes an extra dimension 'S' to appear in dims.

[BrianWhitneyAI]

Diving into this error we discovered the following:

Image dimensions (1, 6, 65, 233, 345) = (T, C, Z, Y, X)

with 2.10.5 using DefaultReader class's guess: (390, 1, 1, 233, 345)
with 2.10.5 using Reader class's guess not DefaultReader's: (1, 1, 390, 233, 345)
with 2.26.1 using DefaultReader class's guess: (6, 1, 1, 65, 233, 345)
with 2.26.1 using Reader class's guess not DefaultReader's: (1, 6, 65, 233, 345)

When reading tiff images, data is stored as stacks of x and y pixels, and future dimensions are dictated by metadata. With imageio 2.10.5 these additional dimensions were stacked in the Time dimension. however, with newer versions this stack is split up into additional dimensions, likely indicating some new metadata reading from imageio versioning >= 2.11.0. As a result of this, we have updated tests to reflect this expected output in DefaultReader using the most recent imageio versioning.

Image dimensions (6, 1, 1, 65, 233, 345) = (T, C, Z, Y, X, S)

[evamaxfield]

I will give this a larger look later but @BrianWhitneyAI those dims don't make sense to me:

Image dimensions (6, 1, 1, 65, 233, 345) = (T, C, Z, Y, X, S)

that means that there are 345 dimensions of RGB....{insert more colors here}

S stands for "Sample" which is typically either 1D (for greyscale), 3D (for RGB), and 4D (for RGBA / CYMK [maybe -- never seen CYMK before in one of our images])

[evamaxfield]

I am also just dropping myself into a PR without much knowledge so if i am completely off-base just let me know haha

[toloudis]

I think I agree with Eva. We should get it to return
Image dimensions (6, 1, 65, 233, 345) = (T, C, Z, Y, X) if at all possible? Assuming imageio itself does not think there are 345 values per pixel.

[BrianWhitneyAI]

Hey! Here's a little more (context/my thoughts) on the issue.

From my understanding, the DefaultReader is a fallback for when no other reader is capable and is intended is specific to less complex, fewer dimension file types. It guesses an order of dimensions defined by _guess_dim_order function in DefaultReader.

Since the change of dimensions comes from _guess_dim_order’s interpretation of a set of dimensions we wanted to maintain this interpretation within the tests to cover a wide range of files.
With newer versions of imageio the dimensions fed to our tests from an ome.tiff changed due to some additional metadata reading. This is not guaranteed for all images, so we decided that the test would for the time being reflect the dimensions asserted by imageio with our unchanged guess of dimensions. All be it not a very good guess…

Some possible paths to improve/resolve this are:

1. Adjusting the _guess_dim_order function in DefaultReader to better accommodate larger dimensionality. Currently, a lot of assumptions are being made about the expected order of dims which leads to the misplaces ‘S’ that we are seeing.
2. Assuming that a cache all guess the function of dim order is too abstract and have DefaultReader inherit the _guess_dim_order function from Reader.
3. Remove the test in question in favor of a similar test that does not try an ome.tiff and deem the DefaultReader as not supported for ome.tiff

[BrianWhitneyAI]

Update! we decided that a simple fix for this is something between 1 and 2. If a 4D image's last dimension is small (<=4) it is deemed 'S' whereas larger ranges inherit the Reader classes _guess_dim_order.

[evamaxfield]

Looks good to me! Thank you for taking the time to solve this!!