Closed tcompa closed 10 months ago
First proposed solution:
wavelength_id
to all existing entries in the omero.channels
list.label
(if it exists)_1
, _2
, ...Broader question:
omero
block is missing? Should we create one by scratch?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
- 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
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.
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
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'}]
EDIT: I'm transforming this into a to-do list to reflect ongoing work in #579:
c
axis (e.g. for a ZYX image).color
attributes if missing;window
attribute (see https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/578#issuecomment-1770991835)update_omero_metadata: bool = True
The proposal sounds good!
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
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.
The proposal sounds good!
FYI, a minor change is needed, to handle the cases where wavelength_id
already exists. All examples in https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/578#issuecomment-1770273286 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.
Agreed, that should be supported - but I think it cannot come before https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/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 https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/150.
Makes sense, let's throw an error for now then!
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
.
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", ..
).window
attribute for nowFor 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]
That's great! Adding colormap, but not Window makes sense. I'll review the PR next, but already 👍🏻 on the overall approach
At the moment we don't add anything.
The test with BIA data has
label
, but notwavelength_id
: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 rightOmeroChannel
object - see traceback below. Note that the issue is not identifying the channel (which I think could be done correctly via thelabel
), but it is with this blockwhich cannot cast data into
OmeroChannel
objects becausewavelength_id
is required.