-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
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')
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
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. |
If anyone can please give this an OK, I would merge it afterwards, no high-impact changes here |
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? |
True, we would then have to update dependencies as well. I think we are moving in that direction for the next release, right? |
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? |
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 |
I broke this PR into 2.
This PR should remain open until we are sure we do not break |
As of napari 0.5.0 this is now an error:
It would be great if this fix gets into a release soon. |
OK now this is a bug with napari>=0.5.0. The workaround until this is fixed is to use a lower napari version... |
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: Let us know if you have any questions about the new API! |
Oh nice @jni ! I will give this a try! Thanks! |
With the current state of this PR, there is no need to check for the napari version, it should work whether the I tried to install this version on top of the latest
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:
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. @jo-mueller, if you agree, could you merge this? |
Agreed @zoccoler, thanks for the fix! |
It became available in 0.4.19. We should probably start adding |
You are right, my mistake! I had looked into napari API for other versions to check that. 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 😌 |
This is a small PR that addresses #307 and fixes a couple minor bugs