add omero metadata to writer

[giovp]

close #260 .

Validation before write is minimal, feedbacks welcome.

[will-moore]

Thanks for the PR. I wonder if you would also want to extend the validation a bit, although some of this would be duplicating the schema validation e.g:

• check that the window has min, max, start, end numbers
• check that the color is a string

You could even check that the number of omero 'channels' matches the number of channels in the data, although this is a bit more work since you have to handle different axes for different versions to know which is channel dimension. Could be left for now.

[will-moore]

Hi @giovp - if you could merge in origin/master branch (or rebase onto it) then that should fix some of the broken jobs, thanks

[giovp]

hi @will-moore apologies for getting this staled, I hope I will be able to work on it sometime next week. Will rebase as well, and keep you posted, sorry for delay.

[giovp]

@will-moore am back on this and will be able to work on it. I have a question re the overwrite of names in the zarr group.
https://github.com/giovp/ome-zarr-py/blob/58bdf584f10d14c74ed80b9a6ab4a53a58943216/ome_zarr/reader.py#L291

is this desired? what's the logic behind?
thank you!

[will-moore]

I think the logic there is if you've got no channels then the layer name in napari will be e.g. "myImage" but if you've got channels with names then they will be used for layers instead, "DAPI", "GFP" etc.

[giovp]

thanks a lot for the prompt reply, from the ngff specs though seems there can be both? a name and channel names? so why would that be overwritten by Omero reader?

https://ngff.openmicroscopy.org/latest/#omero-md

[will-moore]

I think this is because the logic in ome-zarr-py was originally built as a napari plugin, so it was designed just to present the best name for each channel.
If you've only got a multiscales name, napari_ome_zarr uses that for each layer:

But if you've got an omero metadata, we can provide better layer names:

But we only have a single place to store that data: node.metadata["name"], so we have to use it for both cases.

It would certainly be an improvement for ome-zarr-py to store both the multiscales name and the omero channel names, so that both are available to all tools - and then napari-ome-zarr could just pick from either to create layer names.

[giovp]

But we only have a single place to store that data: node.metadata["name"], so we have to use it for both cases.

It would certainly be an improvement for ome-zarr-py to store both the multiscales name and the omero channel names, so that both are available to all tools - and then napari-ome-zarr could just pick from either to create layer names.

ok great, so would it be ok to keep

node.metadata["name"] = multiscales[0].get("name")

as name of image and use a different one here: https://github.com/giovp/ome-zarr-py/blob/58bdf584f10d14c74ed80b9a6ab4a53a58943216/ome_zarr/reader.py#LL390C13-L390C42
like

node.metadata["channel_names"] = names

?

[will-moore]

Yes, node.metadata["channel_names"] = names would make sense.
That would be considered a breaking change to ome-zarr-py so it would justify bumping the version, to 0.7.0 I guess; I don't think we're ready to go to 1.0.0 yet.
Then we'd need a corresponding release of napari-ome-zarr that had dependency of ome-zarr-py 0.7.0 and updated logic for reading the channel_names.

[will-moore]

Tests are failing with:

>       if isinstance(metadata, dict) and "omero" in metadata["metadata"]:
E       KeyError: 'metadata'

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
ome_zarr/data.py	87.00% <100.00%> (-0.13%)	⬇️
ome_zarr/reader.py	86.70% <100.00%> (-3.97%)	⬇️
ome_zarr/writer.py	94.46% <82.35%> (-0.82%)	⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[will-moore]

Failing (only on python 3.8!) with:

    def test_omero_metadata(self, metadata: Optional[dict[str, Any]]):
E   TypeError: 'type' object is not subscriptable

I think this just needs to be dict -> Dict

[giovp]

thanks a lot for prompt feedback and help @will-moore !

Just wanted to clarify one thing regarding metadata: atm, neither color nor windows are mandatory , but if they are present then they should adhere to the types you outlined here: #261 (comment)

therefore, just to recap the specs, they would be as follow:

• metadata MAY contains "omero"
• if it does, then it MUST be a "dict" with either "color" or "window", and not be None
• if it contains "color" then value MUST be string
• if it contains "window" then value MUST be another dict with min, max, start, end with respective values being int.

is this correct? do you think it'd be worth to update the ngff specs to clarify this?
If this is correct I also plan to create PR in: https://github.com/JaneliaSciComp/pydantic-ome-ngff

[will-moore]

Apologies for the nit-pick comment. Otherwise I'm happy with this.
@joshmoore @sbesson - any 2nd opinion, or OK for me to merge once I'm happy with it?

[giovp]

no problem at all, modified accordingly. Just reminder that this concerns the python implementation of this minor specs change ome/ngff#202

[will-moore]

thanks - I'm away just now but happy for @joshmoore or @sbesson to merge

[giovp]

bump

[will-moore]

Just noticed that with elif here, you'll only test the color validation if there is no window metadata.
Just use if "color".. instead.
But also I don't see that if len(color_metadata) < 6: below is ever true, since all the color in parametrize above are length 6.

[giovp]

@will-moore apologies getting back to this only now but it should be fix

[will-moore]

Hi @giovp - thanks for the update...
Tests are failing with:

    def test_omero(self):
        reader = Reader(parse_url(str(self.path)))()
        image_node = list(reader)[0]
>       omero = image_node.load(Multiscales).zarr.root_attrs.get("omero")
E       NameError: name 'Multiscales' is not defined

Also the Codespell / Check for spelling errors should be fixed by #317 so you can fix that by merging in master branch again.

Thanks!

[giovp]

hi @will-moore , test should be fixed, passes locally, sorry again for getting back to you so late

[will-moore]

Great, thanks - apologies for the delay. All tests passing now 👍

[giovp]

looks like it can be merged?

[will-moore]

@giovp yes, apologies we missed this at the last release.
@joshmoore You OK to merge this?

[joshmoore]

Anything else otherwise I'll push out a quick 0.8.4

[will-moore]

Thanks Josh - No I don't see any others ready to go. There have been a couple of docs and tox/deps PRs merged recently.
I noticed some build failures https://github.com/ome/ome_zarr_test_suite/actions/workflows/main.yml but I'm not sure why yet.

[joshmoore]

ewwww

RecursionError: maximum recursion depth exceeded in __instancecheck__

on ome_zarr info

[will-moore]

I can't work out exactly where that's coming from or how to reproduce it locally

[will-moore]

I wonder if the recursion error is related to similar recursion errors noted at #339