Skip to content
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

Adjusting tiffile versioning #487

Merged
merged 30 commits into from
May 1, 2023
Merged

Adjusting tiffile versioning #487

merged 30 commits into from
May 1, 2023

Conversation

BrianWhitneyAI
Copy link
Collaborator

@BrianWhitneyAI BrianWhitneyAI commented Apr 11, 2023

Tifffile 2023.3.15 is released;

2023.3.15

  • Fix corruption using tile generators with prediction/compression.
  • Add parser for Micro-Manager MMStack series (breaking).
  • Return micromanager_metadata IndexMap as numpy array (breaking).
  • Revert optimizations for Micro-Manager OME series.
  • Do not use numcodecs zstd in write_fsspec (kerchunk issue 317).

More type annotations.

“For mmstack images”

TiffFile Before:

  • Individual scenes were accessed through tifffile.series[scene_index]
  • Scene dims = “TCZYX” or “TZCYX” depending on reader.

TiffFile After:

  • Individual scenes are stored in dims. “TRCZYX” or “TRZCYX”.
  • Tifffile.series[scene_index] does not return single scene anymore.
  • Image data is stored at tiffile.series[0].

This change results in our _read_delayed and _read_immediate functions in ome_tiff_reader and tiff_reader being unable to read single scenes.

Discussed Possible solutions:

  1. Creating a psudo-scene object to wrap the scene object and function as normally. Reshaping the internal data when reading initially.
  2. Adding “if mmstack” to each reference of the images traits to reassign dims/shape metadata.
  3. Reshaping data after the object has been created

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.07 ⚠️

Comparison is base (bed6166) 94.06% compared to head (08e3fc4) 93.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   94.06%   93.99%   -0.07%     
==========================================
  Files          48       48              
  Lines        4363     4363              
==========================================
- Hits         4104     4101       -3     
- Misses        259      262       +3     
Impacted Files Coverage Δ
...eaders/extra_readers/test_ome_tiled_tiff_reader.py 100.00% <ø> (ø)
aicsimageio/readers/ome_tiff_reader.py 93.44% <100.00%> (ø)
aicsimageio/readers/tiff_glob_reader.py 86.77% <100.00%> (ø)
aicsimageio/readers/tiff_reader.py 95.21% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@BrianWhitneyAI
Copy link
Collaborator Author

BrianWhitneyAI commented Apr 26, 2023

During the span of this PR, Two additional dependency issues came up. First, the codecov package is no longer supported and was removed from setup.py. Additionally, the pillow plugin in imageio package broke tests with new version 2.28.0 and has been pinned to 2.27.0.

@BrianWhitneyAI BrianWhitneyAI marked this pull request as ready for review April 26, 2023 20:34
@toloudis
Copy link
Collaborator

Pretty sure the codecov thing was resolved in a prior PR and already taken care of in main branch. (it was noted here https://allencellscience.slack.com/archives/CK1L3Q2NP/p1681324180304249 )

@BrianWhitneyAI
Copy link
Collaborator Author

BrianWhitneyAI commented Apr 26, 2023

Pretty sure the codecov thing was resolved in a prior PR and already taken care of in main branch. (it was noted here https://allencellscience.slack.com/archives/CK1L3Q2NP/p1681324180304249 )

Oh yep, after a merge from main this PR no longer covers that issue.

Copy link
Collaborator

@SeanLeRoy SeanLeRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change to tests I'd like to see but besides that looks good!!!

@@ -214,22 +214,10 @@ def test_ome_tiff_reader_large_files(
@pytest.mark.parametrize(
"filename, expected_reader",
[
(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These still seem like worthwhile tests, consider moving them to their respective test files (like the first one here, TiffReader, to the TiffReader test file)

Copy link
Collaborator Author

@BrianWhitneyAI BrianWhitneyAI Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe they are already there! They're under test-selected_tiff_reader in test_tiff_reader and test_ome_tiff_reader.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I wonder why they were here then. Well if no one else has an issue removing them here I guess it is fine!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like they were intentionally added to every reader that handles tiff files, maybe just to ensure that the code that selects which reader to use doesn't get confused. This is kind of like an integration test that makes sense when all the readers are present. Maybe there's a better way to organize the tests so we don't have to have redundant ones like that.

@@ -31,8 +31,8 @@ def run(self):
# "READER_TO_INSTALL" lookup table from aicsimageio/formats.py.
format_libs: Dict[str, List[str]] = {
"base-imageio": [
"imageio[ffmpeg]>=2.11.0",
"Pillow>=9.3",
"imageio[ffmpeg]>=2.11.0,<2.28.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you meant by Pillow plug-in your in Slack message now. Bummer that this is now also an issue :( seems like it can be handled outside of this PR though

Copy link
Collaborator

@toloudis toloudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Not sure what the deal is with imageio/Pillow, but apart from that this seems like the best way to keep current with tifffile. Some time in the future we may revisit those micromanager files with a special reader or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants