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

Simplify colormaps #31

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

GenevieveBuckley
Copy link
Collaborator

Closes #28

Replace most colormap code with inbuilt napari colormap functions.

@GenevieveBuckley GenevieveBuckley mentioned this pull request Jun 14, 2024
@GenevieveBuckley GenevieveBuckley requested a review from Czaki June 14, 2024 06:22
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.

Based on fast code check, the napari dependency could be removed, as if colormap is passed by name, then napari will convert string to colormap instance.

Same with passing colormap as array. It will be properly handled by napari and converted into colormap instance.

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
napari_tiff/napari_tiff_metadata.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
napari_tiff/napari_tiff_metadata.py Outdated Show resolved Hide resolved
@GenevieveBuckley
Copy link
Collaborator Author

the napari dependency could be removed, as if colormap is passed by name, then napari will convert string to colormap instance.

I don't think this is actually true.
As far as I can tell there is no equivalent for the alpha colormap used here. That's the constraint requiring napari.utils.colormap. I'd be delighted if there was a way to avoid this problem.

The original version of this code use vispy colormaps, but I suspect that was part of the cause for bug #28. I'm hoping napari is better able to recognise its own colormaps and avoid duplicates.

@GenevieveBuckley
Copy link
Collaborator Author

EDIT: ah, there is a second place besides the alpha_colormap function where the Colormap class is still used, around line ~155-ish. Maybe there's a way to remove this too

colormap = Colormap(colormap.astype("float32").T)

@Czaki
Copy link
Contributor

Czaki commented Jun 17, 2024

As far as I can tell there is no equivalent for the alpha colormap used here. That's the constraint requiring napari.utils.colormap. I'd be delighted if there was a way to avoid this problem.

So maybe add alpha colormap to napari? It is short to 0.5.0 release. Or just add colormap as [(0,0,0,0), (0,0,0,1)]?

The original version of this code use vispy colormaps, but I suspect that was part of the cause for bug #28. I'm hoping napari is better able to recognise its own colormaps and avoid duplicates.

As far as I know, it is not better. The best way to avoid duplicates is to provide a named colormap. Then napari will check duplicates by name.

@Czaki
Copy link
Contributor

Czaki commented Jun 17, 2024

EDIT: ah, there is a second place besides the alpha_colormap function where the Colormap class is still used, around line ~155-ish. Maybe there's a way to remove this too

colormap = Colormap(colormap.astype("float32").T)

but there could be only array passed to napari. And napari will convert it to colormap.

@GenevieveBuckley
Copy link
Collaborator Author

but there could be only array passed to napari. And napari will convert it to colormap.

This won't get rid of the problem of duplicate colormaps being generated, will it? To be clear, it also sounds like passing an explicit napari Colormap object also still gives us this duplicate colormap problem, unlike what I'd assumed earlier.

@GenevieveBuckley
Copy link
Collaborator Author

As far as I can tell there is no equivalent for the alpha colormap used here. That's the constraint requiring napari.utils.colormap. I'd be delighted if there was a way to avoid this problem.

So maybe add alpha colormap to napari? It is short to 0.5.0 release.

Might have to ask if there's any interest in this.

Or just add colormap as [(0,0,0,0), (0,0,0,1)]?

Passing colormap=[(0,0,0,0), (0,0,0,1)] in directly produces an error. whereas passing colormap=Colormap([(0,0,0,0), (0,0,0,1)]) does not.

That's not too surprising though, looking at the docs

colormap : str, napari.utils.Colormap, tuple, dict, list
    Colormaps to use for luminance images. If a string must be the name
    of a supported colormap from vispy or matplotlib. If a tuple the
    first value must be a string to assign as a name to a colormap and
    the second item must be a Colormap. If a dict the key must be a
    string to assign as a name to a colormap and the value must be a
    Colormap. If a list then must be same length as the axis that is
    being expanded as channels, and each colormap is applied to each
    new image layer.

@GenevieveBuckley
Copy link
Collaborator Author

GenevieveBuckley commented Jun 18, 2024

Passing colormap=[(0,0,0,0), (0,0,0,1)] in directly produces an error. whereas passing colormap=Colormap([(0,0,0,0), (0,0,0,1)]) does not.

Ah, but it does work if you pass it in as a numpy array.

@Czaki
Copy link
Contributor

Czaki commented Jun 18, 2024

For me, reading code like page.photometric == 3 is hard, but tifffile export PHOTOMETRIC class which is Enum and could be used as named constraints to increase code readability.

I still do not have an idea how to deduplicate colormap created from page.colormap. We may need to do this on napari side as I do not found any tiff tag that could contain useful information.

napari_tiff/napari_tiff_metadata.py Show resolved Hide resolved
@GenevieveBuckley
Copy link
Collaborator Author

I don't think we can hold this open any longer for additional reviewers, so I'm going to merge these changes now.

@GenevieveBuckley GenevieveBuckley merged commit 7dd6c3b into napari:main Jul 9, 2024
7 checks passed
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.

Bug: a new colormap is being created every time a tiff image is opened
2 participants