napari / napari-svg

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

Use napari colormaps (allowing non-vispy and custom ones) #29

Closed brisvag closed 1 year ago

brisvag commented 1 year ago

Switch from vispy colormap to napari colormaps for mapping. This is needed for napari/napari#5782 to work. On the flipside, napari/napari#5805 is neede for this PR, because napari's Colormap.map() is currently broken.

I'm currently not sure how to get this in a way that does not break backward compatibility, yet allowing forward compatibillity for when we finally release the aformenentioned changes from napari. We might just have to do a quick release of this plugin right after napari to ensure it's good.

brisvag commented 1 year ago

I ended up removing the fallback and simply pinning napari version. This however means that tests will fail here because the next napari version is not out yet.

brisvag commented 1 year ago

@Czaki do you have some suggestion on how to handle this version bump? In short:

We could simply release this one despite failing CI (which works locally) so it's ready for napari 0.4.18? A bit icky though.

codecov[bot] commented 1 year ago

Codecov Report

Merging #29 (6596b24) into master (1808797) will increase coverage by 0.04%. The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   96.12%   96.17%   +0.04%     
==========================================
  Files           7        7              
  Lines         465      471       +6     
==========================================
+ Hits          447      453       +6     
  Misses         18       18              
Impacted Files Coverage Δ
napari_svg/layer_to_xml.py 96.17% <83.33%> (-0.10%) :arrow_down:
napari_svg/_tests/test_write_layer.py 98.73% <100.00%> (+0.18%) :arrow_up:
brisvag commented 1 year ago

@Czaki could you add some comments about the BIN_APROX part? I don't quite get the reasoning.

brisvag commented 1 year ago

So, tests are all passing locally on the current napari main, after napari/napari#5805 was merged. I dont' think there's a better way to test this, so I would say we can merge!

We should then wait and publish a new version just before 0.4.18, so we minimize the number of people ending up with the BINS_APROX colormap trick (though this is probably totally invisible to users).

brisvag commented 1 year ago

Ok, confirm this still passes both before and after napari/napari#5805. Merging!