diff --git a/CHANGELOG.md b/CHANGELOG.md index 9da60a1f..acebc69a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## [v1.1.3] + +### Fixes + +- Channel windows could land on disk with `start == end`. `compute_image_percentile` returned a degenerate window for a constant, binary, sparse or empty channel: the percentile window now opens to the channel max when the percentiles coincide, then to `(0, value)` for a constant channel (a binary mask gets `(0, 1)`), and an empty channel gets `(0, 1)`. Such a window is spec-legal but useless to a viewer, and napari refuses to open it ([#74](https://github.com/BioVisionCenter/ngio/issues/74)). +- `ChannelVisualisation` no longer silently rewrites `end` to `start + 1` when the two are equal. The repair ran at model construction, so it never reached the disk through `set_channel_windows` (built with `model_copy`, which skips validators) while a file that already carried `(7, 7)` read back as `(7, 8)`: ngio reported a window the disk did not hold. The model now reflects the disk. Instead, `set_channel_windows` warns (`NgioUserWarning`) when a `start`/`end` or `min`/`max` pair is not strictly increasing and writes the values as given, since the spec puts no order on them. + ## [v1.1.2] ### Fixes diff --git a/src/ngio/images/_image.py b/src/ngio/images/_image.py index 2fe944ac..89762ae5 100644 --- a/src/ngio/images/_image.py +++ b/src/ngio/images/_image.py @@ -1,6 +1,7 @@ """Generic class to handle Image-like data in a OME-NGFF file.""" import logging +import warnings from collections.abc import Mapping, Sequence from typing import Any, Literal @@ -37,6 +38,7 @@ from ngio.ome_zarr_meta.ngio_specs import ( Channel, ChannelsMeta, + ChannelVisualisation, DefaultSpaceUnit, DefaultTimeUnit, NgffVersions, @@ -45,6 +47,7 @@ ) from ngio.ome_zarr_meta.ngio_specs._axes import AxesSetup from ngio.utils import ( + NgioUserWarning, NgioValueError, StoreOrGroup, ZarrGroupHandler, @@ -689,6 +692,10 @@ def set_channel_windows( These values are used by viewers to set the display range of each channel. + The spec puts no order on `start`/`end` or `min`/`max`, so any value is + written as given, but a pair that is not strictly increasing is useless + to a viewer and napari refuses it, so it warns. + Args: starts_ends (Sequence[tuple[float, float]]): The start and end values for each channel. @@ -713,10 +720,16 @@ def set_channel_windows( # opens the image and reloads its metadata, and it is the same value # already bound above. for se, mm, ch in zip(starts_ends, min_max_, current_channels, strict=True): + _warn_if_not_increasing("start/end", ch.label, se) updates = {"start": se[0], "end": se[1]} if mm is not None: + _warn_if_not_increasing("min/max", ch.label, mm) updates.update({"min": mm[0], "max": mm[1]}) - channel_visualisation = ch.channel_visualisation.model_copy(update=updates) + # `model_validate`, not `model_copy(update=...)`: a copy skips the + # validators, so what lands on disk would never be checked. + channel_visualisation = ChannelVisualisation.model_validate( + {**ch.channel_visualisation.model_dump(), **updates} + ) channel = ch.model_copy( update={"channel_visualisation": channel_visualisation} ) @@ -919,6 +932,24 @@ def get( ) +def _warn_if_not_increasing(what: str, label: str, pair: tuple[float, float]) -> None: + """Warn when a channel window pair is not strictly increasing. + + The OME-NGFF spec puts no order on the pair, so it is written as given; + but it is useless to a viewer and napari refuses to open it. + """ + low, high = pair + if high > low: + return + warnings.warn( + f"Channel {label!r}: {what} = ({low}, {high}) is not strictly " + "increasing. The spec allows it and the value is written as given, but " + "viewers cannot use it and napari refuses to open it.", + NgioUserWarning, + stacklevel=3, + ) + + def compute_image_percentile( image: Image, percentiles: tuple[float, float] | list[tuple[float, float]] = (0.1, 99.9), @@ -933,6 +964,11 @@ def compute_image_percentile( Returns: A tuple containing the start and end percentiles for each channel. + + Note: + Viewers need `start < end`. When the percentiles coincide (sparse or + constant data) the window opens to the channel max, then to `0`; an + empty channel gets `(0, 1)`. """ num_channels = image.num_channels # handle the case where a single tuple is provided @@ -967,15 +1003,22 @@ def compute_image_percentile( data = data[mask] _data = data.compute() if _data.size == 0: - starts_and_ends.append((0.0, 0.0)) + starts_and_ends.append((0.0, 1.0)) continue # compute the percentiles _s_perc, _e_perc = da.percentile( data, [start_percentile, end_percentile], method="nearest" ).compute() - - starts_and_ends.append((float(_s_perc), float(_e_perc))) + start, end = float(_s_perc), float(_e_perc) + if end <= start: + # Sparse data: the nearest percentiles coincide. Open to the max. + end = float(_data.max()) + if end <= start: + # Constant channel: show the value against the zero background. + # Every masked value is > 0, so this is always a valid window. + start = 0.0 + starts_and_ends.append((start, end)) return starts_and_ends diff --git a/src/ngio/ome_zarr_meta/ngio_specs/_channels.py b/src/ngio/ome_zarr_meta/ngio_specs/_channels.py index 23857bee..6086a742 100644 --- a/src/ngio/ome_zarr_meta/ngio_specs/_channels.py +++ b/src/ngio/ome_zarr_meta/ngio_specs/_channels.py @@ -9,7 +9,7 @@ from typing import Any, TypeVar import numpy as np -from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from ngio.utils import NgioValidationError, NgioValueError @@ -156,20 +156,6 @@ def validate_color(cls, value: str | NgioColors) -> str: else: raise NgioValueError(f"Invalid color {value}.") - @model_validator(mode="before") - def check_start_end(cls, data): - """Check that the start and end values are valid. - - If the start and end values are equal, set the end value to start + 1 - """ - start = data.get("start", None) - end = data.get("end", None) - if start is None or end is None: - return data - if abs(end - start) < 1e-6: - data["end"] = start + 1 - return data - @classmethod def default_init( cls, diff --git a/tests/unit/images/test_image_api.py b/tests/unit/images/test_image_api.py index 4af78597..01f0b10b 100644 --- a/tests/unit/images/test_image_api.py +++ b/tests/unit/images/test_image_api.py @@ -1,5 +1,7 @@ """Coverage tests for channel metadata and channel selection in ngio images.""" +from typing import Any + import dask.array as da import numpy as np import pytest @@ -9,7 +11,7 @@ from ngio import create_empty_ome_zarr from ngio.images import ChannelSelectionModel from ngio.ome_zarr_meta.ngio_specs import Channel, ChannelsMeta -from ngio.utils import NgioValueError +from ngio.utils import NgioUserWarning, NgioValueError def _make_container(): @@ -172,3 +174,86 @@ def test_resolve_channel_selection(): image.resolve_channel_selection("not_a_channel") with pytest.raises(NgioValueError): image.resolve_channel_selection(["DAPI", 7]) + + +def _raw_windows(store) -> list[tuple[float, float]]: + """Read the channel windows from the on-disk JSON, bypassing ngio's models.""" + import zarr + + attrs: dict[str, Any] = dict(zarr.open_group(store, mode="r").attrs) + channels = attrs["omero"]["channels"] + return [(c["window"]["start"], c["window"]["end"]) for c in channels] + + +@pytest.mark.parametrize( + ("data", "expected"), + [ + # empty: no nonzero data at all + (np.zeros((64, 64), dtype="uint16"), (0.0, 1.0)), + # constant: percentiles and max coincide, drop the start to 0 + (np.full((64, 64), 7, dtype="uint16"), (0.0, 7.0)), + # binary mask: the nonzero data is constant + (np.indices((64, 64)).sum(axis=0) % 2, (0.0, 1.0)), + # float constant + (np.full((64, 64), 0.3, dtype="float32"), (0.0, np.float32(0.3))), + ], +) +def test_percentile_window_never_degenerate(data, expected): + """Viewers need start < end; the raw JSON must never carry start == end.""" + store = MemoryStore() + ome_zarr = create_empty_ome_zarr( + store, + shape=(1, 64, 64), + axes_names=("c", "y", "x"), + pixelsize=1.0, + levels=1, + dtype=str(data.dtype), + ) + ome_zarr.get_image().set_array(data[None]) + ome_zarr.set_channel_windows_with_percentiles() + (window,) = _raw_windows(store) + assert window[0] < window[1] + assert window == pytest.approx(expected) + + +def test_percentile_window_sparse_opens_to_max(): + """A single outlier is beyond the 99.9th percentile: the window opens to it.""" + data = np.ones((1, 64, 64), dtype="uint8") + data[0, 3, 5] = 2 + store = MemoryStore() + ome_zarr = create_empty_ome_zarr( + store, + shape=data.shape, + axes_names=("c", "y", "x"), + pixelsize=1.0, + levels=1, + dtype="uint8", + ) + ome_zarr.get_image().set_array(data) + ome_zarr.set_channel_windows_with_percentiles() + assert _raw_windows(store) == [(1.0, 2.0)] + + +def test_set_channel_windows_warns_but_writes_as_given(): + """A non-increasing pair is spec-legal: warn, but write exactly what was asked.""" + store = MemoryStore() + ome_zarr = create_empty_ome_zarr( + store, + shape=(2, 8, 8), + axes_names=("c", "y", "x"), + pixelsize=1.0, + levels=1, + ) + with pytest.warns(NgioUserWarning, match="start/end = \\(5.0, 5.0\\)"): + ome_zarr.set_channel_windows(starts_ends=[(5.0, 5.0), (0.0, 1.0)]) + with pytest.warns(NgioUserWarning, match="start/end = \\(3.0, 1.0\\)"): + ome_zarr.set_channel_windows(starts_ends=[(0.0, 1.0), (3.0, 1.0)]) + assert _raw_windows(store) == [(0.0, 1.0), (3.0, 1.0)] + with pytest.warns(NgioUserWarning, match="min/max = \\(7, 7\\)"): + ome_zarr.set_channel_windows( + starts_ends=[(0.0, 1.0), (0.0, 1.0)], min_max=[(7, 7), (0, 255)] + ) + # The model reflects the disk: no silent repair on read either. + (bad, good) = ome_zarr.get_image().channels_meta.channels + assert (bad.channel_visualisation.min, bad.channel_visualisation.max) == (7, 7) + assert (good.channel_visualisation.min, good.channel_visualisation.max) == (0, 255)