Include Omero-channels-metadata update in import-ome-zarr task

[tcompa]

At the moment we don't add anything.

The test with BIA data has label, but not wavelength_id:

{
  "multiscales" : [ {
    "metadata" : {
      "method" : "loci.common.image.SimpleImageScaler",
      "version" : "Bio-Formats 6.13.0"
    },
    "axes" : [ {
      "name" : "t",
      "type" : "time"
    }, {
      "name" : "c",
      "type" : "channel"
    }, {
      "unit" : "micrometer",
      "name" : "z",
      "type" : "space"
    }, {
      "unit" : "micrometer",
      "name" : "y",
      "type" : "space"
    }, {
      "unit" : "micrometer",
      "name" : "x",
      "type" : "space"
    } ],
    "name" : "WD1_15-02_WT_confocalonly.tif",
    "datasets" : [ {
      "path" : "0",
      "coordinateTransformations" : [ {
        "scale" : [ 1.0, 1.0, 0.5002026153846154, 0.04298806423500096, 0.04298806423500096 ],
        "type" : "scale"
      } ]
    }, {
      "path" : "1",
      "coordinateTransformations" : [ {
        "scale" : [ 1.0, 1.0, 0.5002026153846154, 0.08597612847000193, 0.08597612847000193 ],
        "type" : "scale"
      } ]
    } ],
    "version" : "0.4"
  } ],
  "omero" : {
    "channels" : [ {
      "color" : "808080",
      "coefficient" : 1,
      "active" : true,
      "label" : "Channel 0",
      "window" : {
        "min" : 0.0,
        "max" : 133.0,
        "start" : 0.0,
        "end" : 133.0
      },
      "family" : "linear",
      "inverted" : false
    } ],
    "rdefs" : {
      "defaultT" : 0,
      "model" : "greyscale",
      "defaultZ" : 18
    }
  }
}

If we now run import-ome-zarr an then cellpose-segmentation, with channel=Channel(label="Channel 0"), get_channel_from_image_zarr fails in getting back the right OmeroChannel object - see traceback below.
Note that the issue is not identifying the channel (which I think could be done correctly via the label), but it is with this block

def get_omero_channel_list(*, image_zarr_path: str) -> list[OmeroChannel]:
    group = zarr.open_group(image_zarr_path, mode="r+")
    channels_dicts = group.attrs["omero"]["channels"]
    channels = [OmeroChannel(**c) for c in channels_dicts]
    return channels

which cannot cast data into OmeroChannel objects because wavelength_id is required.

---

        # Find channel index
        try:
>           tmp_channel: OmeroChannel = get_channel_from_image_zarr(
                image_zarr_path=zarrurl,
                wavelength_id=channel.wavelength_id,
                label=channel.label,
            )

../../fractal_tasks_core/tasks/cellpose_segmentation.py:288: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def get_channel_from_image_zarr(
        *,
        image_zarr_path: str,
        label: Optional[str] = None,
        wavelength_id: Optional[str] = None,
    ) -> OmeroChannel:
        """
        Extract a channel from OME-NGFF zarr attributes.
    
        This is a helper function that combines `get_omero_channel_list` with
        `get_channel_from_list`.
    
        Args:
            image_zarr_path: Path to an OME-NGFF image zarr group.
            label: `label` attribute of the channel to be extracted.
            wavelength_id: `wavelength_id` attribute of the channel to be
                extracted.
    
        Returns:
            A single channel dictionary.
        """
>       omero_channels = get_omero_channel_list(image_zarr_path=image_zarr_path)

../../fractal_tasks_core/lib_channels.py:186: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def get_omero_channel_list(*, image_zarr_path: str) -> list[OmeroChannel]:
        """
        Extract the list of channels from OME-NGFF zarr attributes.
    
        Args:
            image_zarr_path: Path to an OME-NGFF image zarr group.
    
        Returns:
            A list of channel dictionaries.
        """
        group = zarr.open_group(image_zarr_path, mode="r+")
        channels_dicts = group.attrs["omero"]["channels"]
>       channels = [OmeroChannel(**c) for c in channels_dicts]

../../fractal_tasks_core/lib_channels.py:205: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <list_iterator object at 0x7f0ce49328c0>

>   channels = [OmeroChannel(**c) for c in channels_dicts]

../../fractal_tasks_core/lib_channels.py:205: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   pydantic.error_wrappers.ValidationError: 1 validation error for OmeroChannel
E   wavelength_id
E     field required (type=value_error.missing)

pydantic/main.py:341: ValidationError

[tcompa]

First proposed solution:

1. import-ome-zarr adds wavelength_id to all existing entries in the omero.channels list.
2. The value is made of the value of label (if it exists)
3. What if labels are not unique across the list? Probably we have to add something like _1, _2, ...

Broader question:
4. What if the whole omero block is missing? Should we create one by scratch?

[jluethi]

That makes sense :)

Yes, let's go with this first implementation as proposed by you, i.e. have it use label (if it exists) with optional suffixes if it's non-unique.
If there is no label, let's just use numbers for both label and wavelength id, e.g.
label: 1
label: 2
label: 3
For the 3 channels, same for wavelength id

4. What if the whole omero block is missing? Should we create one by scratch?

If we can reasonably create that block, then yes. I'd leave out Window (that is allowed now by all our models, right?). Not 100% sure anymore whether viewers handle it well when no color is set

[tcompa]

Something else: should we expose this feature through a update_omero_metadata: bool = False task parameter?

I'd be in favor, since it results into a change of an existing zarr.

[jluethi]

Yes, that sounds good. But I'd make it default True. Because the default behavior should be: You use the import OME-Zarr task, you now expect this Zarr file to be Fractal compatible.

If it's false, we could log warnings if there are issues? Or if that results in duplicated / complex code flow: We just log when it's active so that there is some way of knowing what gets added

[tcompa]

Here is a proposal for the expected behavior of the "update existing Omero channels" feature - see below.

If we are OK with this behavior, I'll proceed and look at the other aspects of the current issue (which are not part of this comment) -- see next comment.

# Easy case 1: input has no labels
OLD: [{}, {}, {}]
NEW: [{'label': '1', 'wavelength_id': '1'}, {'label': '2', 'wavelength_id': '2'}, {'label': '3', 'wavelength_id': '3'}]

# Easy case 2: input has unique labels
OLD: [{'label': 'A'}, {'label': 'B'}, {'label': 'C'}]
NEW: [{'label': 'A', 'wavelength_id': 'A'}, {'label': 'B', 'wavelength_id': 'B'}, {'label': 'C', 'wavelength_id': 'C'}]

# Easy case 3: input has unique labels, when they are present
OLD: [{'label': 'A'}, {}, {'label': 'C'}]
NEW: [{'label': 'A', 'wavelength_id': 'A'}, {'label': '2', 'wavelength_id': '2'}, {'label': 'C', 'wavelength_id': 'C'}]

# Easy case 4: input has the same label everywhere
OLD: [{'label': 'A'}, {'label': 'A'}, {'label': 'A'}]
NEW: [{'label': 'A', 'wavelength_id': 'A'}, {'label': 'A', 'wavelength_id': 'A-1'}, {'label': 'A', 'wavelength_id': 'A-2'}]

# Case 5: input has the same label everywhere, and it is a number
OLD: [{'label': '1'}, {'label': '1'}, {'label': '1'}]
NEW: [{'label': '1', 'wavelength_id': '1'}, {'label': '1', 'wavelength_id': '1-1'}, {'label': '1', 'wavelength_id': '1-2'}]

# Case 6: input has a mix of presence/absence of labels, and they may be a number
OLD: [{}, {'label': '1'}, {}, {'label': '1'}]
NEW: [{'label': '1', 'wavelength_id': '1-2'}, {'label': '1', 'wavelength_id': '1'}, {'label': '3', 'wavelength_id': '3'}, {'label': '1', 'wavelength_id': '1-1'}]

[tcompa]

EDIT: I'm transforming this into a to-do list to reflect ongoing work in #579:

• Take a decision on expected behavior described in Include Omero-channels-metadata update in import-ome-zarr task #578 (comment)
• Raise an error the NGFF image has no c axis (e.g. for a ZYX image).
• Add color attributes if missing;
• Do nothing re: window attribute (see Include Omero-channels-metadata update in import-ome-zarr task #578 (comment))
• When the Omero channel list is not present, create it from scratch (based on the number of channels in the zarr array).
• When the Omero channel list exists in the NGFF metadata, but with a different number of channels from the ones in the Zarr array, raise an informative error. Note: I think in this case it'd be impossible to know which Zarr-array channel matches to a given NGFF-Omero channel.
• Encapsulate all these new features in an optional block, in the import-ome-zarr task, depending on a boolean task parameter update_omero_metadata: bool = True
• Make sure logging is appropriate, for all the several branches of this new feature

[jluethi]

The proposal sounds good!

[jluethi]

When the NGFF image has no c axis (e.g. for a ZYX image), what should we do? Current status: we raise an informative error.

Hmm, even for a single channel image? In that case, that should eventually be supported. But we can also add that later on

[tcompa]

Hmm, even for a single channel image? In that case, that should eventually be supported. But we can also add that later on

Agreed, that should be supported - but I think it cannot come before #150.

As far as I understand, we don't support ZYX Zarrs in fractal-tasks-core. Therefore the only option I see is that we'd have to modify the Zarr array, and increase its shape from e.g. 3 axes (ZYX) to 4 axes (CZYX), with the C axis having dimension 1. This means changing the actual data, on top of the NGFF metadata, and IMO it doesn't fit into the "import-ome-zarr" task scope. It would rather fit in a "convert-to-fractal-zarr" task, but then again this would become obsolete with #150.

[tcompa]

The proposal sounds good!

FYI, a minor change is needed, to handle the cases where wavelength_id already exists. All examples in #578 (comment) remain the same, but a new one is also included

# Case 7: a mix of missing/existing label and/or wavelength values
OLD: [{'wavelength_id': '1'}, {'label': '1'}, {}, {'label': '1', 'wavelength_id': '3'}]
NEW: [{'wavelength_id': '1', 'label': '1'}, {'label': '1', 'wavelength_id': '1-1'}, {'label': '3', 'wavelength_id': '3-1'}, {'label': '1', 'wavelength_id': '3'}]

The basic principle here is that we always keep existing label or wavelength_id values, and then (with lower priority) we assign all the others.

[jluethi]

Agreed, that should be supported - but I think it cannot come before #150.

As far as I understand, we don't support ZYX Zarrs in fractal-tasks-core. Therefore the only option I see is that we'd have to modify the Zarr array, and increase its shape from e.g. 3 axes (ZYX) to 4 axes (CZYX), with the C axis having dimension 1. This means changing the actual data, on top of the NGFF metadata, and IMO it doesn't fit into the "import-ome-zarr" task scope. It would rather fit in a "convert-to-fractal-zarr" task, but then again this would become obsolete with #150.

Makes sense, let's throw an error for now then!

[tcompa]

Regarding color and window attributes

I produced an NGFF image Zarr, with omero metadata

"omero": {
        "channels": [
            {
                "label": "1",
                "wavelength_id": "1"
            }
        ]
    }

and tried to load it with napari-ome-zarr.

Here is the error traceback (with additional debugging info I added):

venv/lib/python3.10/site-packages/napari_ome_zarr/_reader.py:151 transform.<locals>.f
    metadata: {
        'scale': (
            1.0,
            0.1625,
            0.1625,
        ),
        'channel_axis': 0,
        'name': ['1'],
        'visible': [True],
        'contrast_limits': [None],
        'colormap': [],
    } (dict) len=6
Traceback (most recent call last):
  File "/tmp/venv/lib/python3.10/site-packages/napari/layers/utils/stack_utils.py", line 140, in split_channels
    i_kwargs[key] = next(val)
StopIteration

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/tmp/venv/bin/napari", line 8, in <module>
    sys.exit(main())
  File "/tmp/venv/lib/python3.10/site-packages/napari/__main__.py", line 564, in main
    _run()
  File "/tmp/venv/lib/python3.10/site-packages/napari/__main__.py", line 340, in _run
    viewer._window._qt_viewer._qt_open(
  File "/tmp/venv/lib/python3.10/site-packages/napari/_qt/qt_viewer.py", line 953, in _qt_open
    self.viewer.open(
  File "/tmp/venv/lib/python3.10/site-packages/napari/components/viewer_model.py", line 1092, in open
    self._add_layers_with_plugins(
  File "/tmp/venv/lib/python3.10/site-packages/napari/components/viewer_model.py", line 1318, in _add_layers_with_plugins
    added.extend(self._add_layer_from_data(*_data))
  File "/tmp/venv/lib/python3.10/site-packages/napari/components/viewer_model.py", line 1392, in _add_layer_from_data
    layer = add_method(data, **(meta or {}))
  File "/tmp/venv/lib/python3.10/site-packages/napari/utils/migrations.py", line 59, in _update_from_dict
    return func(*args, **kwargs)
  File "/tmp/venv/lib/python3.10/site-packages/napari/components/viewer_model.py", line 902, in add_image
    layerdata_list = split_channels(data, channel_axis, **kwargs)
  File "/tmp/venv/lib/python3.10/site-packages/napari/layers/utils/stack_utils.py", line 142, in split_channels
    raise IndexError(
IndexError: Error adding multichannel image with data shape (1, 2, 2160, 5120).
Requested channel_axis (0) had length 1, but the 'colormap' argument only provided 0 values. 

while the image is displayed correctly if I add the property:

    "color": "FFFFFF",

even in the absence of window.

TLDR

1. I will now add a bit of logic to always include a color property (if missing) in omero channels, as part of import-ome-zarr. I will copy the logic of define_omero_channels (that is, using "00FFFF", "FF00FF", "FFFF00", "808080", "808080", "808080", ..).
2. I will not add a window attribute for now

[tcompa]

For the record, here is the visual difference between including (first screenshot) or not including (second screenshot) a reasonable choice of the window property (while keeping the other properties the same):

[napari 0.4.18, napari-ome-zarr 0.5.2, ome-zarr 0.8.2]

[jluethi]

That's great! Adding colormap, but not Window makes sense. I'll review the PR next, but already 👍🏻 on the overall approach