napari-clusters-plotter icon indicating copy to clipboard operation
napari-clusters-plotter copied to clipboard

Fix color future warning

Open zoccoler opened this issue 10 months ago • 7 comments

This is a small PR that addresses #307 and fixes a couple minor bugs

zoccoler avatar Mar 26 '24 14:03 zoccoler

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.47%. Comparing base (ba6a572) to head (7cdf261).

Files Patch % Lines
napari_clusters_plotter/_plotter.py 66.66% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   79.46%   79.47%           
=======================================
  Files          16       16           
  Lines        2075     2076    +1     
=======================================
+ Hits         1649     1650    +1     
  Misses        426      426           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 26 '24 14:03 codecov[bot]

If anyone can please give this an OK, I would merge it afterwards, no high-impact changes here

zoccoler avatar Mar 26 '24 14:03 zoccoler

so in general everything LGTM but this only works for environments with napari >= 0.4.19 so there is a backward compatibility breaking change I guess we should mention?

Cryaaa avatar Mar 26 '24 15:03 Cryaaa

True, we would then have to update dependencies as well. I think we are moving in that direction for the next release, right?

zoccoler avatar Mar 27 '24 15:03 zoccoler

That is true but maybe that would create issues with the devbio-napari system... @haesleinhuepf: do you know if relying on a specific napari version would cause issues for the devbio napari install?

Cryaaa avatar Mar 27 '24 15:03 Cryaaa

Let's wait for @haesleinhuepf feedback on this, it can be halted since it is not a major fix.

I would like to eventually have it implemented (maybe it can wait a couple months or so), because it does impact a little my usage of the current plotter by the napari-flim-phasor-plotter plugin

zoccoler avatar Mar 28 '24 09:03 zoccoler

I broke this PR into 2.

  • here, only changes needed to fix the FutureWarning will be kept
  • the other minor changes were moved to #313

This PR should remain open until we are sure we do not break devbio-napari installation.

zoccoler avatar Apr 12 '24 14:04 zoccoler

As of napari 0.5.0 this is now an error:

TypeError: _add_layer_from_data received an unexpected keyword argument ('color') for layer type labels

It would be great if this fix gets into a release soon.

ziw-liu avatar Jul 22 '24 16:07 ziw-liu

OK now this is a bug with napari>=0.5.0.

The workaround until this is fixed is to use a lower napari version...

zoccoler avatar Jul 29 '24 06:07 zoccoler

this only works for environments with napari >= 0.4.19 so there is a backward compatibility breaking change

You can check the napari installed version and pass one key or the other based on that, which is what we did with napari-ome-zarr, here:

https://github.com/ome/napari-ome-zarr/pull/112/files#diff-3ac23664593f6255d26806df61d2364f3d3f0b66c53a4fad561a70877093239aR134-R139

Let us know if you have any questions about the new API!

jni avatar Aug 06 '24 09:08 jni

You can check the napari installed version and pass one key or the other based on that, which is what we did with napari-ome-zarr, here:

https://github.com/ome/napari-ome-zarr/pull/112/files#diff-3ac23664593f6255d26806df61d2364f3d3f0b66c53a4fad561a70877093239aR134-R139

Oh nice @jni ! I will give this a try! Thanks!

zoccoler avatar Aug 09 '24 09:08 zoccoler

With the current state of this PR, there is no need to check for the napari version, it should work whether the Labels layer has the .color attribute or not because the colors dictionary is always converted with DirectLabelColormap (only available with napari>=0.4.18 TMK) into a colormap and assigned to the .colormap attribute.

I tried to install this version on top of the latest devbio-napari version (0.10.1) but I could not make it work when installing with pip becasue it brings npe>=0.7.0,

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
devbio-napari 0.10.1 requires npe2<0.7.0, but you have npe2 0.7.7 which is incompatible.

This seems to be a requirement of napari>=0.5.1. For example, if I try to downgrade npe2 to 0.6.2, I get:

- package napari-base-0.5.1-pyh9208f05_0 requires npe2 >=0.7.6

So, napari-clusters-plotter may still work if installed with mamba/conda along with devbio-napari, but we can only test that after generating a new recipe in conda-forge (TMK).

I suggest we merge changes here regardless of the problems above because otherwise this is a bug of this plugin when napari>=0.5.0. If this new version impacts devbio-napari installation, we should create an issue there and work on a solution from the outside, but not here.

@jo-mueller, if you agree, could you merge this?

zoccoler avatar Aug 12 '24 14:08 zoccoler

Agreed @zoccoler, thanks for the fix!

jo-mueller avatar Aug 13 '24 06:08 jo-mueller

the colors dictionary is always converted with DirectLabelColormap (only available with napari>=0.4.18 TMK) into a colormap

It became available in 0.4.19. We should probably start adding .. versionadded:: directives to docstrings when we add new things... 😅🙏

jni avatar Aug 13 '24 06:08 jni

the colors dictionary is always converted with DirectLabelColormap (only available with napari>=0.4.18 TMK) into a colormap

It became available in 0.4.19. We should probably start adding .. versionadded:: directives to docstrings when we add new things... 😅🙏

You are right, my mistake! I had looked into napari API for other versions to check that. DirectLabelColormap only "shows up" in 0.4.19 (https://napari.org/0.4.19/api/index.html), thanks for the correction!

We have 0.4.19 as a requirement here (https://github.com/BiAPoL/napari-clusters-plotter/blob/44a658789fc119e339e320e4cab10238d67781eb/setup.cfg#L52C5-L52C19) so it should all be fine 😌

zoccoler avatar Aug 13 '24 06:08 zoccoler