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

Fix color future warning #308

Merged
merged 10 commits into from
Aug 13, 2024
Merged

Fix color future warning #308

merged 10 commits into from
Aug 13, 2024

Conversation

zoccoler
Copy link
Collaborator

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

zoccoler and others added 4 commits March 26, 2024 14:54
Replaces .color property of Labels layer by assigning colormap with DirectLabelColormap to match napari 0.5.0
An Attribution Error raises if the viewer is closed from code because somehow an Image layer can be present at self.layer_select.value, which has no 'properties' property
This reverts opacity attribution to the layer (the value of the combobox), not to the combobox (which is 'layer_select')
Copy link

codecov bot commented Mar 26, 2024

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 ⚠️
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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoccoler
Copy link
Collaborator Author

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

@Cryaaa
Copy link
Collaborator

Cryaaa commented Mar 26, 2024

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?

@zoccoler
Copy link
Collaborator Author

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

@Cryaaa
Copy link
Collaborator

Cryaaa commented Mar 27, 2024

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?

@zoccoler
Copy link
Collaborator Author

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
Copy link
Collaborator Author

zoccoler commented Apr 12, 2024

I broke this PR into 2.

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

@ziw-liu
Copy link

ziw-liu commented Jul 22, 2024

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.

@zoccoler
Copy link
Collaborator Author

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

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

@jni
Copy link

jni commented Aug 6, 2024

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!

@zoccoler
Copy link
Collaborator Author

zoccoler commented Aug 9, 2024

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 zoccoler removed the on hold label Aug 12, 2024
@zoccoler
Copy link
Collaborator Author

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?

@jo-mueller
Copy link
Collaborator

Agreed @zoccoler, thanks for the fix!

@jo-mueller jo-mueller merged commit 44a6587 into main Aug 13, 2024
8 checks passed
@jo-mueller jo-mueller deleted the fix_color_future_warning branch August 13, 2024 06:37
@jni
Copy link

jni commented Aug 13, 2024

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... 😅🙏

@zoccoler
Copy link
Collaborator Author

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 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants