Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/ndevio/bioio_plugins/_compatibility.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
"""Compatibility checks for known bioio reader format limitations.

These functions inspect reader state after initialisation and emit a single
consolidated warning when a known incompatibility is detected, so that
downstream property accessors can fail silently without repeating noisy
messages.
"""

from __future__ import annotations

import logging
from typing import TYPE_CHECKING

logger = logging.getLogger(__name__)

if TYPE_CHECKING:
from bioio_ome_zarr import Reader as OmeZarrReader


def warn_if_old_zarr_format(reader: OmeZarrReader) -> None:
"""Emit one warning if *reader* is a ``bioio_ome_zarr.Reader`` for a v0.1/v0.2 store.

``bioio_ome_zarr.Reader`` unconditionally accesses ``coordinateTransformations``
inside each ``datasets`` entry — a key introduced in OME-Zarr v0.3. Any call
to ``reader.scale`` or ``reader.dimension_properties`` therefore raises
``KeyError`` for v0.1/v0.2 stores.

``nImage`` silently falls back to ``scale=1.0`` / ``units=None`` in those
paths, but this warning gives users a single, clear explanation upfront via
the napari activity log.

Parameters
----------
reader : OmeZarrReader
"""
multiscales = reader._multiscales_metadata
datasets = multiscales[0].get('datasets', []) if multiscales else []
if datasets and 'coordinateTransformations' not in datasets[0]:
version = multiscales[0].get('version', 'unknown (likely 0.1 or 0.2)')
Comment on lines +36 to +39
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warn_if_old_zarr_format reads reader._multiscales_metadata directly. Because this is a private attribute, it may not exist (or may be renamed) across bioio_ome_zarr versions, which would raise AttributeError during nImage initialization. Consider using getattr(reader, '_multiscales_metadata', None) (and returning early when missing) so this compatibility helper never introduces a new hard failure path.

Suggested change
multiscales = reader._multiscales_metadata
datasets = multiscales[0].get('datasets', []) if multiscales else []
if datasets and 'coordinateTransformations' not in datasets[0]:
version = multiscales[0].get('version', 'unknown (likely 0.1 or 0.2)')
multiscales = getattr(reader, "_multiscales_metadata", None)
if not multiscales:
# Older or incompatible bioio_ome_zarr versions may not expose this
# private attribute. In that case, silently skip this compatibility
# warning rather than introducing a new hard failure path.
return
datasets = multiscales[0].get("datasets", []) or []
if not datasets:
return
if "coordinateTransformations" not in datasets[0]:
version = multiscales[0].get("version", "unknown (likely 0.1 or 0.2)")

Copilot uses AI. Check for mistakes.
logger.warning(
'OME-Zarr compatibility warning: this store appears to be '
'OME-Zarr spec version %s, which pre-dates '
"'coordinateTransformations' in dataset entries (introduced "
'in v0.3). Physical scale and unit metadata cannot be read. '
'ndevio will open the image with scale=1.0 and no units. '
'Consider converting the file to OME-Zarr >=v0.4.',
version,
)
18 changes: 14 additions & 4 deletions src/ndevio/nimage.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ def __init__(
self.path = None
self._is_remote = False

# Any compatibility warnings for old formats should be emitted at this point
# Cheaply check without imports by looking at the reader's module name
if self.reader.__module__.startswith('bioio_ome_zarr'):
from .bioio_plugins._compatibility import warn_if_old_zarr_format

warn_if_old_zarr_format(self.reader)

@property
def reference_xarray(self) -> xr.DataArray:
"""Image data as xarray DataArray for metadata determination.
Expand Down Expand Up @@ -363,12 +370,13 @@ def layer_scale(self) -> tuple[float, ...]:
axis_labels = self.layer_axis_labels

# Try to get scale from BioImage - may fail for array-like inputs
# where physical_pixel_sizes is None
# where physical_pixel_sizes is None (AttributeError), or for old OME-Zarr formats
# (v0.1/v0.2) that lack 'coordinateTransformations' in their dataset
# metadata (introduced in v0.3) (KeyError).
try:
bio_scale = self.scale
except AttributeError:
except (AttributeError, KeyError):
return tuple(1.0 for _ in axis_labels)
Comment on lines +378 to 379
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching KeyError here unconditionally will also hide unexpected KeyErrors from non-OME-Zarr readers (or other metadata issues), silently forcing a default scale. To avoid masking real problems, consider only swallowing KeyError when the active reader is a bioio_ome_zarr reader (and/or when the old-format guard has detected missing coordinateTransformations), otherwise re-raise.

Suggested change
except (AttributeError, KeyError):
return tuple(1.0 for _ in axis_labels)
except AttributeError:
# No scale information available for this image; fall back to 1.0
return tuple(1.0 for _ in axis_labels)
except KeyError:
# Only swallow KeyError for known old OME-Zarr metadata layouts.
# For other readers, re-raise to avoid hiding real metadata issues.
reader = getattr(self, "_reader", None)
reader_module = getattr(reader, "__module__", "")
if "bioio_ome_zarr" in reader_module:
return tuple(1.0 for _ in axis_labels)
raise

Copilot uses AI. Check for mistakes.

return tuple(
getattr(bio_scale, dim, None) or 1.0 for dim in axis_labels
)
Expand Down Expand Up @@ -419,7 +427,9 @@ def layer_units(self) -> tuple[str | None, ...]:

try:
dim_props = self.dimension_properties
except AttributeError:
# Old OME-Zarr v0.1/v0.2: dimension_properties → reader.scale →
# _get_scale_array raises KeyError for missing 'coordinateTransformations'.
except (AttributeError, KeyError):
return tuple(None for _ in axis_labels)

Comment on lines +432 to 434
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching KeyError here for all readers will silently drop units for any unexpected KeyError path. Consider narrowing the KeyError fallback to only the known old OME-Zarr case (e.g., gate on reader type/module or a flag set by the compatibility check) and re-raise for other readers so genuine metadata bugs aren’t hidden.

Suggested change
except (AttributeError, KeyError):
return tuple(None for _ in axis_labels)
except AttributeError:
# No dimension_properties available at all – no units can be resolved.
return tuple(None for _ in axis_labels)
except KeyError as e:
# Narrow KeyError handling to legacy OME-Zarr readers only.
reader = getattr(self, "_reader", None)
module_name = type(reader).__module__ if reader is not None else ""
if (
"omezarr" in module_name
or "ome_zarr" in module_name
or "zarr" in module_name
):
# Known legacy OME-Zarr behavior: missing coordinateTransformations.
# Fall back to no units for all axes.
return tuple(None for _ in axis_labels)
# For all other readers, re-raise to avoid hiding genuine metadata bugs.
raise e

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, maybe this is something to consider for the future, but it adds considerable complexity now. Maybe if folks open issues when their metadata isn't being read as expected I can investigate more

def _get_unit(dim: str) -> str | None:
Expand Down
138 changes: 138 additions & 0 deletions tests/test_bioio_plugins/test_compatibility.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
"""Tests for bioio_plugins._compatibility module."""

from __future__ import annotations

import logging
from unittest.mock import MagicMock, patch


def _make_zarr_reader(multiscales: list) -> MagicMock:
"""Return a mock that looks like a ``bioio_ome_zarr.Reader`` instance."""
reader = MagicMock()
reader.__module__ = 'bioio_ome_zarr.reader'
reader._multiscales_metadata = multiscales
return reader


def _make_v01_multiscales(version: str = '0.1') -> list:
"""Minimal OME-Zarr v0.1/v0.2 multiscales — no coordinateTransformations."""
return [{'version': version, 'datasets': [{'path': '0'}, {'path': '1'}]}]


def _make_v03_multiscales() -> list:
"""Minimal OME-Zarr >=v0.3 multiscales — has coordinateTransformations."""
return [
{
'version': '0.3',
'datasets': [
{
'path': '0',
'coordinateTransformations': [
{'type': 'scale', 'scale': [1.0, 0.5, 0.5]}
],
}
],
}
]


class TestWarnIfOldZarrFormat:
"""Unit tests for warn_if_old_zarr_format."""

def test_v01_emits_warning(self, caplog):
"""v0.1 metadata (no coordinateTransformations) triggers a warning."""
from ndevio.bioio_plugins._compatibility import warn_if_old_zarr_format

reader = _make_zarr_reader(_make_v01_multiscales('0.1'))

with caplog.at_level(
logging.WARNING, logger='ndevio.bioio_plugins._compatibility'
):
warn_if_old_zarr_format(reader)

assert len(caplog.records) == 1
assert '0.1' in caplog.records[0].message
assert 'coordinateTransformations' in caplog.records[0].message
assert 'scale=1.0' in caplog.records[0].message

def test_v02_emits_warning(self, caplog):
"""v0.2 metadata also triggers a warning."""
from ndevio.bioio_plugins._compatibility import warn_if_old_zarr_format

reader = _make_zarr_reader(_make_v01_multiscales('0.2'))

with caplog.at_level(
logging.WARNING, logger='ndevio.bioio_plugins._compatibility'
):
warn_if_old_zarr_format(reader)

assert len(caplog.records) == 1
assert '0.2' in caplog.records[0].message

def test_v03_no_warning(self, caplog):
"""v0.3+ metadata (has coordinateTransformations) emits no warning."""
from ndevio.bioio_plugins._compatibility import warn_if_old_zarr_format

reader = _make_zarr_reader(_make_v03_multiscales())

with caplog.at_level(
logging.WARNING, logger='ndevio.bioio_plugins._compatibility'
):
warn_if_old_zarr_format(reader)

assert len(caplog.records) == 0

def test_empty_multiscales_no_warning(self, caplog):
"""Empty multiscales list does not raise and emits no warning."""
from ndevio.bioio_plugins._compatibility import warn_if_old_zarr_format

reader = _make_zarr_reader([])

with caplog.at_level(
logging.WARNING, logger='ndevio.bioio_plugins._compatibility'
):
warn_if_old_zarr_format(reader)

assert len(caplog.records) == 0

def test_unknown_version_in_warning(self, caplog):
"""When version key is missing the warning still fires with a fallback string."""
from ndevio.bioio_plugins._compatibility import warn_if_old_zarr_format

# No 'version' key, no 'coordinateTransformations'
multiscales = [{'datasets': [{'path': '0'}]}]
reader = _make_zarr_reader(multiscales)

with caplog.at_level(
logging.WARNING, logger='ndevio.bioio_plugins._compatibility'
):
warn_if_old_zarr_format(reader)

assert len(caplog.records) == 1
assert 'unknown' in caplog.records[0].message.lower()


class TestNImageCompatibilityGuard:
"""Integration tests: nImage.__init__ only calls warn_if_old_zarr_format for zarr readers."""

def test_non_zarr_reader_skips_check(self, resources_dir):
"""A TIFF-backed nImage never calls warn_if_old_zarr_format."""
from ndevio import nImage

with patch(
'ndevio.bioio_plugins._compatibility.warn_if_old_zarr_format'
) as mock_check:
nImage(resources_dir / 'cells3d2ch_legacy.tiff')

mock_check.assert_not_called()

def test_zarr_reader_calls_check(self, resources_dir):
"""A zarr-backed nImage calls warn_if_old_zarr_format exactly once."""
from ndevio import nImage

with patch(
'ndevio.bioio_plugins._compatibility.warn_if_old_zarr_format'
) as mock_check:
nImage(resources_dir / 'dimension_handling_zyx_V3.zarr')

mock_check.assert_called_once()
15 changes: 15 additions & 0 deletions tests/test_nimage.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,21 @@ def test_nImage_remote_zarr():
assert img.reference_xarray.shape == (2, 512, 512)


@pytest.mark.network
def test_nImage_remote_zarr_old_format(caplog):
"""Test that nImage emits a warning for old OME-Zarr formats when reading remotely."""
remote_zarr = 'https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.1/9836841.zarr' # from https://github.com/ndev-kit/ndevio/issues/50
with caplog.at_level(
'WARNING', logger='ndevio.bioio_plugins._compatibility'
):
img = nImage(remote_zarr)
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test configures caplog for the compatibility logger but never asserts that a warning was actually emitted, so it will still pass if the warning regresses or is removed. Add an assertion on caplog.records (filtered to ndevio.bioio_plugins._compatibility) to validate the intended warning behavior.

Suggested change
img = nImage(remote_zarr)
img = nImage(remote_zarr)
# ensure a warning was emitted from the compatibility logger
compatibility_warnings = [
record
for record in caplog.records
if record.name == 'ndevio.bioio_plugins._compatibility'
and record.levelname == 'WARNING'
]
assert compatibility_warnings, "Expected a WARNING from ndevio.bioio_plugins._compatibility when reading old OME-Zarr format"

Copilot uses AI. Check for mistakes.
assert img.path == remote_zarr
# should catch a key error due to old format
# but still quietly create a scale with no units
assert img.layer_scale == (1.0, 1.0)
assert img.layer_units == (None, None)


def test_nImage_ome_reader(resources_dir: Path):
"""
Test that the OME-TIFF reader is used for OME-TIFF files.
Expand Down