napari / napari-svg

A plugin for writing svg files from napari
BSD 3-Clause "New" or "Revised" License
4 stars 12 forks source link

Update shapes #12

Closed kevinyamauchi closed 4 years ago

kevinyamauchi commented 4 years ago

This PR updates the shapes SVG writer. In particular, it adds support for colored shapes and updates the handling of opacity.

I accidentally started the PR here: https://github.com/sofroniewn/napari-svg/pull/1

Closes #6, #10

Testing

To test this, I checked out the PR in a fresh environment, pip installed locally and then installed napari from this PR.

kevinyamauchi commented 4 years ago

I am bringing this test error over from the other PR:

@sofroniewn , when you ran tests before did you get errors from vispy for the image and labels layers?

FAILED napari_svg/_tests/test_write_layer.py::test_write_image_no_extension[image] - KeyError: 'colormap name gray not found'

.venv/lib/python3.7/site-packages/vispy/color/colormap.py:1120: KeyError

I think it might be these lines: https://github.com/sofroniewn/napari-svg/blob/834f7d104065618ae8a71ed78c399379b4404078/napari_svg/layer_to_xml.py#L67-L70

I think it should be grays:

https://github.com/vispy/vispy/blob/bb4606dc6a41832732474a037bbb92628646815d/vispy/color/colormap.py#L1049-L1059

It looks like 'gray' is a matplotlib colormap that is supported by vispy and 'grays' is a vispy-native colormap. It looks like vispy tries to import matplotlib and if it can't, it skips trying to return a matplotlib colormap. Do you think that pytest is unable to import matplotlib for some reason?

From the same environment, I can both import matplotlib from the python terminal and use the svg writer plugin to make a working SVG. Any thoughts @tlambert03 ?

sofroniewn commented 4 years ago

It looks like 'gray' is a matplotlib colormap that is supported by vispy and 'grays' is a vispy-native colormap. It looks like vispy tries to import matplotlib and if it can't, it skips trying to return a matplotlib colormap. Do you think that pytest is unable to import matplotlib for some reason?

Hmm strange, not sure what's going on there. Seems like going with the vispy default is safest for now

kevinyamauchi commented 4 years ago

We can switch it to grays in the test. The default colormap in the Image layer is gray, so we would also have to either pass a colormap argument in with the layer metadata in the tests or change the default in the Image layer. To be honest, I am a little surprised this issue hasn't come up because neither napari nor vispy explicitly depends on matplotlib. I guess matplotlib is just everywhere 😛

I will do a bit more testing of the tests to see if I can definitely pin down the root cause.

On a somewhat-related note, I just realized that calling the non-data parameters of the layer metadata for the plugins is a little confusing given that all of the layers also have a metadata parameter.

sofroniewn commented 4 years ago

We can switch it to grays in the test. The default colormap in the Image layer is gray, so we would also have to either pass a colormap argument in with the layer metadata in the tests or change the default in the Image layer.

Maybe we should switch to grays everywhere, including napari

On a somewhat-related note, I just realized that calling the non-data parameters of the layer metadata for the plugins is a little confusing given that all of the layers also have a metadata parameter.

Indeed, we tend to use meta for the full layer metadata, and then metadata for the dict on each layer. I don't love it, but it's where we are at right now ....

kevinyamauchi commented 4 years ago

Okay, I think the shapes layer writer is working with colors and I have addressed the empty shapes layer and grayscale colormap issue. An SVG generated with the plugin with a layer with colored shapes and an empty shapes layer:

Screenshot 2020-05-25 at 21 13 10

Colored shapes issue (#6 )

Empty shapes layer issue (#10 )

In terms of behavior this means that:

Grayscale colormap issue (#13):

kevinyamauchi commented 4 years ago

@sofroniewn, okay sounds good. I will remove those comments. I am going to bed soon, so I will leave it up in case anybody has any comments and then merge tomorrow.

kevinyamauchi commented 4 years ago

@sofroniewn , it looks like I don't have write access to this repo, so I think you will have to merge it.

sofroniewn commented 4 years ago

Just gave you write access. I will merge now. Should I also make the new release 0.1.3 release or are we waiting on anything else?

kevinyamauchi commented 4 years ago

I just tested this with https://github.com/napari/napari/pull/1248. All of the napari tests pass and the writer is able to handle empty shapes layers, so I think it is good to go. Merging now!

@sofroniewn , it would be great if you could make the 0.1.3 release!