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

Add support for ome pyramid (multi-resolution) files #24

Merged
merged 26 commits into from
Jun 7, 2024

Conversation

folterj
Copy link
Contributor

@folterj folterj commented Mar 19, 2024

Reverted multi-layer code to the original single layer using channel_axis.
Extended ome-tiff testing, with metadata and loading existing files, and testing napari layer/metadata.

@GenevieveBuckley
Copy link
Collaborator

Ok, I think we have too many ideas happening in one PR. I love that you have lots of improvements, but I think we're going to have to break this up into smaller chunks for make it easier for me to review and merge things.

Can we separate the pyramid support from the ome-tiff metadata handling?

@GenevieveBuckley GenevieveBuckley changed the title Dev3 Add support for ome pyramid (multi-resolution) files Mar 26, 2024
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

In general, the code looks good.

For me, the main problem is lack of descriptive comments under if comments to clarify some edge cases.
Also, code should contain direct raising exception not pass 4 colormaps for 5 channels.

Instead of providing default values for some parameters, like scale, blending etc I prefer to provide None here and allow the viewer to handle this (for example adjust scale to already added layers).

Maybe part of my comment are dumb because I do not have proper context.

napari_tiff/napari_tiff_colormaps.py Show resolved Hide resolved
napari_tiff/napari_tiff_colormaps.py Show resolved Hide resolved
napari_tiff/napari_tiff_colormaps.py Show resolved Hide resolved
napari_tiff/napari_tiff_colormaps.py Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Show resolved Hide resolved
napari_tiff/napari_tiff_reader.py Outdated Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Outdated Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Outdated Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Show resolved Hide resolved
@GenevieveBuckley
Copy link
Collaborator

Thank you so much for your contribution @folterj 🎉
This is a really helpful feature to have, especially as there are so many people out there working with multi-resolution files.

I'd also like to apologize to you for both the delay in getting this merged, and for the number of unrelated reviewer comments on this PR.

@GenevieveBuckley GenevieveBuckley merged commit 4ddc195 into napari:main Jun 7, 2024
7 checks passed
This was referenced Jun 7, 2024
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.

3 participants