ome / ome-zarr-py

Implementation of next-generation file format (NGFF) specifications for storing bioimaging data in the cloud.
https://pypi.org/project/ome-zarr
Other
156 stars 54 forks source link

napari 0.5 support #389

Closed jni closed 4 months ago

jni commented 4 months ago

Fixes ome/napari-ome-zarr#109

... or at least, attempts to do so!

@psobolewskiPhD, now with napari/napari#7061 merged and the code in this PR, this doesn't crash anymore, but the labels layer never finishes loading. Do you see the same thing? I'm not sure what the cause could be...

jni commented 4 months ago

I get the expected warning:

$ NAPARI_ASYNC=1 napari https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0001A/2551.zarr/A/1/0
/Users/jni/projects/napari/napari/utils/colormaps/colormap.py:435: UserWarning: color_dict did not provide a default color. Missing keys will be transparent. To provide a default color, use the key `None`, or provide a defaultdict instance.

which confirms that the colormap dict is being correctly passed to napari... But then... Nothing. 😕

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.81%. Comparing base (9205bd7) to head (55e3328). Report is 15 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #389 +/- ## ========================================== + Coverage 85.76% 85.81% +0.05% ========================================== Files 13 13 Lines 1531 1537 +6 ========================================== + Hits 1313 1319 +6 Misses 218 218 ```

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

jni commented 4 months ago

@joshmoore @will-moore I notice the message

TODO: a metadata transform should be provided by specific impls.

right above my changes, which I totally agree with 😅, indeed I was surprised that I had to come to this repo to fix this issue, but since we hope to release 0.5.0 very shortly, I was hoping to merge this fix first, do a release, and then migrate the logic to napari-ome-zarr at our leisure after that release. Any thoughts? 🙏

joshmoore commented 4 months ago

Happy to see a fix & release here, but I agree that this is surprising to find in this repo.

psobolewskiPhD commented 4 months ago

I'm a bit confused, I don't get why this fix is in this repo and not the plugin? Here's what i observe:

  1. the continued loading appears to be due to failure to broadcast -- the Labels layer has a singleton Z dim.
  2. If I scroll to the 0 slice, the loading stops, but no labels are shown.

I find that this simple fix, of dropping the color key in the plugin and "translating" the metadata color key to colormap for napari, seems to work https://github.com/ome/napari-ome-zarr/compare/main...psobolewskiPhD:napari-ome-zarr:drop_color_key?expand=1 Any thoughts? Note: it doesn't solve the broadcast/loading issue, which might be an upstream napari issue.

psobolewskiPhD commented 4 months ago

BTW, i can reproduce the infinite spin without zarr/etc. so that's a napari bug -- maybe just a visual? dunno. Made an issue: https://github.com/napari/napari/issues/7070

jni commented 4 months ago

I find that this simple fix, of dropping the color key in the plugin and "translating" the metadata color key to colormap for napari, seems to work ome/napari-ome-zarr@main...psobolewskiPhD:napari-ome-zarr:drop_color_key?expand=1 (compare) Any thoughts?

(1) the changes should account for both 0.4.19x and for 0.5.0, because passing the dict only works on 0.5.0, unfortunately. (2) I looked at both places but I found that "color" was coming from here and it is not actually an ome-zarr key from the spec, it's a translation from "colors" that I think is only on account of napari? (Based on the other things in that dict.) So I thought, (a) fix the translation and release, then (b) move that whole logic to the plugin.

Am I misunderstanding what the "color" key is doing here @will-moore @joshmoore?

jni commented 4 months ago

Hi @joshmoore @will-moore — napari 0.5.0 will be released in the next hour or so, at which point napari-ome-zarr will be unable to load any datasets with labels + color annotations. I guess that doesn't describe most datasets, but still, it would be good to fix this. 🙏

will-moore commented 4 months ago

Sorry - been away, looking now....

jni commented 4 months ago

nick of time 😃

Screenshot 2024-07-11 at 7 48 38 PM

Thank you! 🙏

will-moore commented 4 months ago

Test setup - new env etc...

$ conda create -y -n napari-env-310 -c conda-forge python=3.10
$ conda activate napari-env-310
$ python -m pip install "napari[all]<0.5.0"

# pip install -e . (from main branches of ome-zarr-py and napari-ome-zarr)

$ napari --plugin napari-ome-zarr https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0076A/10501752.zarr

Working with labels being loaded...

Repeated with a new env using newly released napari and get:

  File "/Users/wmoore/opt/anaconda3/envs/napari5-env-310/lib/python3.10/site-packages/napari/components/viewer_model.py", line 1511, in _add_layer_from_data
    raise TypeError(
TypeError: _add_layer_from_data received an unexpected keyword argument ('color') for layer type labels

With @psobolewskiPhD's fix above in napari-ome-zarr, this is working for me again. @psobolewskiPhD would you like to open a PR with that change? (and remove the now-invalid comment # NB: color for labels... and maybe add a comment at the other change to mention that we're mapping "color -> colormap for napari 0.5.0"

I think the easiest "fix" to make sure this doesn't break napari 0.4 users is to require napari>=0.5.0 in setup.cfg. Is there a reason that users might want to avoid a big upgrade (for a tiny fix like this) or an easy way to avoid this? I guess we can detect the napari version but comparing version numbers etc is a bit of a pain.

jni commented 4 months ago

@will-moore just to clarify, you only need the changes from ome/napari-ome-zarr#112, right? This can be closed?

in this repo, can you clarify — was the color key only for napari's benefit or does it have some other function? (ie should it be removed anyway)

joshmoore commented 4 months ago

@will-moore just to clarify, you only need the changes from ome/napari-ome-zarr#112, right? This can be closed?

Will is away for a couple of weeks but I asked him this as well and it was a yes. I'm less clear on your other question but if you see no blockers, I say we get the other fix out.

jni commented 4 months ago

I do not — let's merge ome/napari-ome-zarr#112 and release!