napari / napari-tiff

A napari reader plugin for tiff images.
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Simplify colormaps #31

Closed GenevieveBuckley closed 2 months ago

GenevieveBuckley commented 2 months ago

Closes https://github.com/napari/napari-tiff/issues/28

Replace most colormap code with inbuilt napari colormap functions.

GenevieveBuckley commented 2 months ago

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 https://github.com/napari/napari-tiff/issues/28. I'm hoping napari is better able to recognise its own colormaps and avoid duplicates.

GenevieveBuckley commented 2 months ago

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 https://github.com/napari/napari-tiff/blob/33a9426a8af24a8e2a79bbed21f788ebbc698928/napari_tiff/napari_tiff_metadata.py#L159

Czaki commented 2 months ago

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 commented 2 months ago

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

https://github.com/napari/napari-tiff/blob/33a9426a8af24a8e2a79bbed21f788ebbc698928/napari_tiff/napari_tiff_metadata.py#L159

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

GenevieveBuckley commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

GenevieveBuckley commented 2 months ago

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