diff --git a/py/ngff_zarr/parse_metadata.py b/py/ngff_zarr/parse_metadata.py index 86db9021..04e8e38d 100644 --- a/py/ngff_zarr/parse_metadata.py +++ b/py/ngff_zarr/parse_metadata.py @@ -144,6 +144,68 @@ def _parse_hcs_path(store_path: str) -> tuple[str, str | None]: return store_path, None +def _raw_axes(multiscales_entry: dict) -> list: + """The raw axis entries of one ``multiscales`` entry, at any version. + + v0.4 and v0.5 carry a flat ``axes`` list on the entry. From v0.6 (RFC-5) + the axes live in a coordinate system, and the intrinsic one is whichever + system the datasets map into, named by their transformation ``output``. + Nothing requires it to be listed first, so it is resolved by name; a + document whose datasets name no output falls back to the first system, + which is what both ports write. + + Returns the list as found, without filtering: an axis may legally be a + dict, and a malformed entry is left for the caller to classify. Returns an + empty list when the entry carries neither shape. + + Every caller that needs the axes of a raw metadata document goes through + here, so a version that moves them again is a one-line change rather than + a silently skipped check (the v0.6 move is what left the RFC-4 checks + unreachable in both the reader and the conformance report). + """ + if not isinstance(multiscales_entry, dict): + return [] + axes = multiscales_entry.get("axes") + if isinstance(axes, list): + return axes + systems = multiscales_entry.get("coordinateSystems") + if not isinstance(systems, list) or not systems: + return [] + intrinsic = _intrinsic_system(multiscales_entry, systems) + if isinstance(intrinsic, dict): + intrinsic_axes = intrinsic.get("axes") + if isinstance(intrinsic_axes, list): + return intrinsic_axes + return [] + + +def _intrinsic_system(multiscales_entry: dict, systems: list): + """The coordinate system the datasets map into, else the first listed. + + Mirrors :attr:`ngff_zarr.v06.zarr_metadata.Metadata.intrinsic_coordinate_system` + at the dict level, where the parsed dataclasses are not available yet. + """ + for dataset in multiscales_entry.get("datasets") or []: + if not isinstance(dataset, dict): + continue + transforms = dataset.get("coordinateTransformations") + if not isinstance(transforms, list) or not transforms: + continue + if not isinstance(transforms[0], dict): + continue + output = transforms[0].get("output") + if not isinstance(output, dict): + continue + name = output.get("name") + if name is None: + continue + for system in systems: + if isinstance(system, dict) and system.get("name") == name: + return system + break + return systems[0] + + def _is_hcs_plate(root_attrs: dict) -> bool: """Check if root attributes indicate an HCS plate structure. diff --git a/py/ngff_zarr/rfc4_conformance.py b/py/ngff_zarr/rfc4_conformance.py index 9191bbfe..663dc0a1 100644 --- a/py/ngff_zarr/rfc4_conformance.py +++ b/py/ngff_zarr/rfc4_conformance.py @@ -25,6 +25,7 @@ from pathlib import Path from typing import Any +from .parse_metadata import _raw_axes from .rfc4_validation import _ANATOMICAL_AXIS_OF # Metadata files that may hold the multiscales, most specific first. @@ -45,20 +46,29 @@ def __init__(self, code: str, message: str) -> None: def _axes_from_metadata(meta: Any) -> list[dict[str, Any]]: - """Pull ``multiscales[0].axes`` out of a v2 ``.zattrs`` or v3 ``zarr.json`` blob.""" + """Pull the axes out of a v2 ``.zattrs`` or v3 ``zarr.json`` blob. + + The axes are located by :func:`~ngff_zarr.parse_metadata._raw_axes`, so a + v0.6 document, whose axes live in the intrinsic coordinate system rather + than in a flat ``axes`` key, yields its axes instead of being classified + as not-OME-Zarr. + """ if not isinstance(meta, dict): raise _UnreadableInput("input-not-ome-zarr", "metadata is not a JSON object") attributes = meta.get("attributes", meta) ome = attributes.get("ome", attributes) if isinstance(attributes, dict) else {} try: - axes = ome["multiscales"][0]["axes"] + entry = ome["multiscales"][0] except (KeyError, IndexError, TypeError) as exc: raise _UnreadableInput( - "input-not-ome-zarr", "metadata has no multiscales[0].axes" + "input-not-ome-zarr", "metadata has no multiscales[0]" ) from exc - if not isinstance(axes, list): + axes = _raw_axes(entry) + if not axes: raise _UnreadableInput( - "input-not-ome-zarr", "multiscales[0].axes is not a list" + "input-not-ome-zarr", + "multiscales[0] carries no axes, in a flat 'axes' list or in " + "'coordinateSystems'", ) return axes diff --git a/py/ngff_zarr/rfc4_validation.py b/py/ngff_zarr/rfc4_validation.py index 07df9a58..863f5864 100644 --- a/py/ngff_zarr/rfc4_validation.py +++ b/py/ngff_zarr/rfc4_validation.py @@ -216,6 +216,41 @@ def validate_rfc4_orientation(axes: list[dict[str, Any]]) -> None: validator.validate(axes_structure) +def has_any_rfc4_orientation(axes: list[dict[str, Any]]) -> bool: + """Whether any axis carries a non-empty ``orientation``. + + :func:`has_rfc4_orientation_metadata` answers the narrower question of + whether a *spatial* axis is oriented, which is what callers deciding + whether to record orientation want. Callers deciding whether to *validate* + want this one: an orientation on a non-spatial axis is itself an RFC 4 + violation, so skipping validation because no spatial axis is oriented is + precisely how that violation goes unreported. + + Only ``None`` and ``{}`` are undefined under RFC 4, which is the pair + :func:`validate_rfc4_orientation` skips. Any other value, a falsey ``[]``, + ``""``, ``0`` or ``False`` included, is malformed rather than absent and so + must reach the validator. + + Parameters + ---------- + axes : List[Dict[str, Any]] + List of axis metadata dictionaries + + Returns + ------- + bool + True if any axis carries an orientation that is neither absent nor empty + """ + for axis in axes: + if not isinstance(axis, dict) or "orientation" not in axis: + continue + orientation = axis["orientation"] + if orientation is None or orientation == {}: + continue + return True + return False + + def has_rfc4_orientation_metadata(axes: list[dict[str, Any]]) -> bool: """ Check if the axes contain RFC 4 anatomical orientation metadata. diff --git a/py/ngff_zarr/structural_validation.py b/py/ngff_zarr/structural_validation.py index 6d2429b6..e6244e99 100644 --- a/py/ngff_zarr/structural_validation.py +++ b/py/ngff_zarr/structural_validation.py @@ -525,14 +525,14 @@ def validate_axis_orientation(metadata: Metadata) -> None: """Validate RFC 4 anatomical-orientation metadata on the spatial axes. This rule does not reimplement RFC 4; it wraps the package's existing logic - -- :func:`~ngff_zarr.rfc4_validation.has_rfc4_orientation_metadata` and + -- :func:`~ngff_zarr.rfc4_validation.has_any_rfc4_orientation` and :func:`~ngff_zarr.rfc4_validation.validate_rfc4_orientation` -- and surfaces its failures through the unified :class:`ValidationError` channel. The parsed :class:`~ngff_zarr.v04.zarr_metadata.Axis` objects are first rendered back to the axis-dict form those helpers expect (see :func:`_axis_to_validation_dict`). - Orientation is optional in RFC 4, so when no spatial axis carries it the rule - is a no-op and the comparatively heavy ``jsonschema`` import inside + Orientation is optional in RFC 4, so when no axis carries one the rule is a + no-op and the comparatively heavy ``jsonschema`` import inside :func:`validate_rfc4_orientation` is never triggered. Raises @@ -549,23 +549,15 @@ def validate_axis_orientation(metadata: Metadata) -> None: vocabulary -- a schema-level concern with no dedicated structural rule. """ from .rfc4_validation import ( - has_rfc4_orientation_metadata, + has_any_rfc4_orientation, validate_rfc4_orientation, ) axes_dicts = [_axis_to_validation_dict(axis) for axis in metadata.axes] - # A stray orientation on a non-spatial axis carries no spatial-axis - # orientation, so has_rfc4_orientation_metadata alone would skip it; check for - # any real (non-empty) orientation so the non-space rule is reachable. A null - # or empty orientation is undefined under RFC 4 and so is not a violation. - non_space_orientation = any( - isinstance(axis_dict, dict) - and axis_dict.get("type") != "space" - and isinstance(axis_dict.get("orientation"), dict) - and axis_dict.get("orientation") - for axis_dict in axes_dicts - ) - if not has_rfc4_orientation_metadata(axes_dicts) and not non_space_orientation: + # Gated on *any* orientation, not just a spatial one: an orientation on a + # non-spatial axis is itself the violation, so a spatial-only check would + # skip exactly the document that needs reporting. + if not has_any_rfc4_orientation(axes_dicts): return try: validate_rfc4_orientation(axes_dicts) diff --git a/py/ngff_zarr/v04/zarr_metadata.py b/py/ngff_zarr/v04/zarr_metadata.py index dd3f4d16..14339012 100644 --- a/py/ngff_zarr/v04/zarr_metadata.py +++ b/py/ngff_zarr/v04/zarr_metadata.py @@ -425,9 +425,9 @@ def _from_zarr_attrs( import packaging.version from ..ngff_image import NgffImage - from ..parse_metadata import _parse_omero + from ..parse_metadata import _parse_omero, _raw_axes from ..rfc4_validation import ( - has_rfc4_orientation_metadata, + has_any_rfc4_orientation, validate_rfc4_orientation, ) from ..validate import validate as validate_ngff @@ -462,17 +462,16 @@ def _from_zarr_attrs( else: validate_ngff(root_attrs, version=schema_version) - # RFC 4 validation for anatomical orientation - if "axes" in root_attrs["multiscales"][0] and isinstance( - root_attrs["multiscales"][0]["axes"], list - ): - # Type cast each axis item to dict for validation - axes_dicts = [] - for axis in root_attrs["multiscales"][0]["axes"]: - if isinstance(axis, dict): - axes_dicts.append(axis) - if axes_dicts and has_rfc4_orientation_metadata(axes_dicts): - validate_rfc4_orientation(axes_dicts) + # RFC 4 validation for anatomical orientation. The axes are read + # through the shared helper, which knows where each version keeps + # them, and a non-dict axis entry is left to the schema check. + axes_dicts = [ + axis + for axis in _raw_axes(root_attrs["multiscales"][0]) + if isinstance(axis, dict) + ] + if axes_dicts and has_any_rfc4_orientation(axes_dicts): + validate_rfc4_orientation(axes_dicts) omero = _parse_omero(root_attrs.get("omero")) # OME-Zarr v0.5 hoists the spec ``version`` to the group-level ``ome`` diff --git a/py/ngff_zarr/v06/zarr_metadata.py b/py/ngff_zarr/v06/zarr_metadata.py index b067a7a3..2b8caacd 100644 --- a/py/ngff_zarr/v06/zarr_metadata.py +++ b/py/ngff_zarr/v06/zarr_metadata.py @@ -661,9 +661,9 @@ def _from_zarr_attrs( import dask.array from ..ngff_image import NgffImage - from ..parse_metadata import _parse_omero + from ..parse_metadata import _parse_omero, _raw_axes from ..rfc4_validation import ( - has_rfc4_orientation_metadata, + has_any_rfc4_orientation, validate_rfc4_orientation, ) from ..validate import validate as validate_ngff @@ -680,17 +680,17 @@ def _from_zarr_attrs( version=root_attrs["ome"]["multiscales"][0].get("version", "0.6"), ) - # RFC 4 validation for anatomical orientation - if "axes" in root_attrs["ome"]["multiscales"][0] and isinstance( - root_attrs["ome"]["multiscales"][0]["axes"], list - ): - # Type cast each axis item to dict for validation - axes_dicts = [] - for axis in root_attrs["ome"]["multiscales"][0]["axes"]: - if isinstance(axis, dict): - axes_dicts.append(axis) - if axes_dicts and has_rfc4_orientation_metadata(axes_dicts): - validate_rfc4_orientation(axes_dicts) + # RFC 4 validation for anatomical orientation. From v0.6 the axes + # live in the intrinsic coordinate system, so they are read through + # the shared helper rather than from a flat ``axes`` key, which a + # v0.6 entry does not carry. + axes_dicts = [ + axis + for axis in _raw_axes(root_attrs["ome"]["multiscales"][0]) + if isinstance(axis, dict) + ] + if axes_dicts and has_any_rfc4_orientation(axes_dicts): + validate_rfc4_orientation(axes_dicts) omero = _parse_omero(root_attrs.get("ome", {}).get("omero")) root_attrs = root_attrs["ome"]["multiscales"][0] diff --git a/py/test/test_cli_conformance.py b/py/test/test_cli_conformance.py index 89316c34..cdcd5e03 100644 --- a/py/test/test_cli_conformance.py +++ b/py/test/test_cli_conformance.py @@ -287,3 +287,62 @@ def test_conformance_cli_prints_json(tmp_path, monkeypatch, capsys): report = json.loads(capsys.readouterr().out) assert report["rfc4_valid"] is True assert report["format"] == "ome-zarr" + + +def _write_zarr_v06(tmp_path: Path, axes: list[Axis]) -> str: + """Write a v0.6-shaped ``zarr.json``: the axes live in the intrinsic system.""" + metadata = { + "attributes": { + "ome": { + "version": "0.6", + "multiscales": [ + { + "coordinateSystems": [{"name": "intrinsic", "axes": axes}], + "datasets": [{"path": "0"}], + } + ], + } + } + } + (tmp_path / "zarr.json").write_text(json.dumps(metadata)) + return str(tmp_path) + + +def test_conformance_report_reads_v06_coordinate_systems(tmp_path): + """A v0.6 document is classified on its axes, not rejected as unreadable. + + Before the axes were read through the shared helper, this shape carried no + flat ``axes`` key, so every v0.6 input -- valid or not -- came back as + ``input-not-ome-zarr`` with an empty axis map. + """ + report = conformance_report(_write_zarr_v06(tmp_path, list(_LPS))) + assert report["rfc4_valid"] is True + assert report["violations"] == [] + assert report["axes"] == { + "z": "inferior-to-superior", + "y": "anterior-to-posterior", + "x": "right-to-left", + } + + +def test_conformance_report_classifies_a_v06_violation(tmp_path): + """A v0.6 violation is reported with its own code, not as unreadable.""" + path = _write_zarr_v06( + tmp_path, + [ + _space("y", "left-to-right"), + _space("x", "right-to-left"), + ], + ) + report = conformance_report(path) + assert report["rfc4_valid"] is False + assert report["violations"] == ["duplicate-anatomical-axis"] + + +def test_conformance_report_without_axes_anywhere(tmp_path): + """An entry carrying neither shape stays an unreadable input.""" + metadata = {"attributes": {"ome": {"multiscales": [{"datasets": []}]}}} + (tmp_path / "zarr.json").write_text(json.dumps(metadata)) + report = conformance_report(str(tmp_path)) + assert report["rfc4_valid"] is False + assert report["violations"] == ["input-not-ome-zarr"] diff --git a/py/test/test_parse_metadata_axes.py b/py/test/test_parse_metadata_axes.py new file mode 100644 index 00000000..4e3d162a --- /dev/null +++ b/py/test/test_parse_metadata_axes.py @@ -0,0 +1,96 @@ +# SPDX-FileCopyrightText: Copyright (c) Fideus Labs LLC +# SPDX-License-Identifier: MIT +"""Tests for the single place that locates the axes in raw metadata. + +``_raw_axes`` is what every reader and the conformance report use to find the +axes of a ``multiscales`` entry, so the version-dependent location is decided +once. The v0.6 case is the one that regressed: the axes moved into the +intrinsic coordinate system, and code that looked only for a flat ``axes`` key +silently found none. +""" + +from ngff_zarr.parse_metadata import _raw_axes + +_AXES = [{"name": "y", "type": "space"}, {"name": "x", "type": "space"}] + + +def test_flat_axes_are_returned(): + assert _raw_axes({"axes": _AXES}) == _AXES + + +def test_v06_axes_come_from_the_intrinsic_coordinate_system(): + entry = {"coordinateSystems": [{"name": "intrinsic", "axes": _AXES}]} + assert _raw_axes(entry) == _AXES + + +def test_a_flat_list_wins_over_coordinate_systems(): + """A document carrying both is read as the flat, pre-v0.6 shape.""" + entry = { + "axes": _AXES, + "coordinateSystems": [{"name": "other", "axes": [{"name": "t"}]}], + } + assert _raw_axes(entry) == _AXES + + +def test_entries_without_axes_yield_an_empty_list(): + assert _raw_axes({"datasets": [{"path": "0"}]}) == [] + assert _raw_axes({"coordinateSystems": []}) == [] + assert _raw_axes({"coordinateSystems": [{"name": "intrinsic"}]}) == [] + assert _raw_axes({"axes": "not a list"}) == [] + assert _raw_axes("not an entry") == [] + + +def test_axis_entries_are_returned_verbatim(): + """Malformed entries are passed through for the caller to classify.""" + entry = {"axes": ["not an object", {"name": 42}]} + assert _raw_axes(entry) == ["not an object", {"name": 42}] + + +def test_the_intrinsic_system_is_resolved_by_name_not_position(): + """Nothing requires the intrinsic system to be listed first. + + The datasets name the system they map into through their transformation + ``output``; reading ``coordinateSystems[0]`` would report another system's + axes, so an invalid intrinsic orientation could pass and an unrelated one + be reported instead. + """ + entry = { + "coordinateSystems": [ + {"name": "physical", "axes": [{"name": "t", "type": "time"}]}, + {"name": "intrinsic", "axes": _AXES}, + ], + "datasets": [ + { + "path": "0", + "coordinateTransformations": [ + { + "input": {"path": "0"}, + "output": {"name": "intrinsic"}, + "type": "scale", + "scale": [1.0, 1.0], + } + ], + } + ], + } + assert _raw_axes(entry) == _AXES + + +def test_an_unresolvable_output_falls_back_to_the_first_system(): + """Both ports write the intrinsic system first, so it is the safe default.""" + entry = { + "coordinateSystems": [{"name": "intrinsic", "axes": _AXES}], + "datasets": [{"path": "0", "coordinateTransformations": []}], + } + assert _raw_axes(entry) == _AXES + + named_elsewhere = { + "coordinateSystems": [{"name": "intrinsic", "axes": _AXES}], + "datasets": [ + { + "path": "0", + "coordinateTransformations": [{"output": {"name": "absent"}}], + } + ], + } + assert _raw_axes(named_elsewhere) == _AXES diff --git a/py/test/test_rfc4_validation.py b/py/test/test_rfc4_validation.py index 1f28a88b..f9d1185c 100644 --- a/py/test/test_rfc4_validation.py +++ b/py/test/test_rfc4_validation.py @@ -569,3 +569,110 @@ def test_from_ngff_zarr_without_rfc4_validation(): # Should succeed without validation multiscales = from_ngff_zarr(store, validate=False) assert multiscales is not None + + +def test_orientation_on_a_non_space_axis_is_reachable(): + """An orientation only on a non-spatial axis is itself the violation. + + Gating validation on a *spatial* orientation would skip exactly the + document the non-space rule exists to catch. + """ + pytest.importorskip("jsonschema", reason="jsonschema required for RFC 4 validation") + + from ngff_zarr.rfc4_validation import has_any_rfc4_orientation + + axes = [ + { + "name": "t", + "type": "time", + "orientation": {"type": "anatomical", "value": "left-to-right"}, + }, + {"name": "y", "type": "space"}, + {"name": "x", "type": "space"}, + ] + assert not has_rfc4_orientation_metadata(axes) + assert has_any_rfc4_orientation(axes) + with pytest.raises(ValueError, match="non-space axes"): + validate_rfc4_orientation(axes) + + +def test_an_empty_orientation_does_not_trigger_validation(): + """A null or empty orientation is undefined under RFC 4, not a violation.""" + from ngff_zarr.rfc4_validation import has_any_rfc4_orientation + + assert not has_any_rfc4_orientation( + [{"name": "y", "type": "space", "orientation": None}] + ) + assert not has_any_rfc4_orientation( + [{"name": "y", "type": "space", "orientation": {}}] + ) + assert not has_any_rfc4_orientation([{"name": "y", "type": "space"}]) + + +@pytest.mark.parametrize("orientation", [[], "", 0, False, "left-to-right"]) +def test_a_falsey_orientation_still_reaches_the_validator(orientation): + """Only ``None`` and ``{}`` are undefined; anything else is malformed. + + A truthiness test would call ``[]``, ``""``, ``0`` and ``False`` absent and + skip validation, but :func:`validate_rfc4_orientation` rejects each of them, + so the gate would hide exactly those documents. + """ + jsonschema = pytest.importorskip( + "jsonschema", reason="jsonschema required for RFC 4 validation" + ) + + from ngff_zarr.rfc4_validation import has_any_rfc4_orientation + + axes = [ + {"name": "y", "type": "space", "orientation": orientation}, + {"name": "x", "type": "space"}, + ] + assert has_any_rfc4_orientation(axes) + with pytest.raises((ValueError, jsonschema.ValidationError)): + validate_rfc4_orientation(axes) + + +def test_read_rejects_orientation_on_a_non_space_axis(): + """The reader reaches the non-space rule with no spatial axis oriented. + + Gating on a spatial orientation left this document accepted: nothing in it + orients a space axis, which is exactly what makes it invalid. + """ + pytest.importorskip("jsonschema", reason="jsonschema required for RFC 4 validation") + + store = MemoryStore() + root = zarr.open_group(store, mode="w") + if hasattr(root, "create_array"): + root.create_array("0", shape=(2, 10, 10), dtype="uint8") + else: + root.create_dataset("0", shape=(2, 10, 10), dtype="uint8") + + root.attrs["multiscales"] = [ + { + "version": "0.4", + "name": "test", + "axes": [ + { + "name": "t", + "type": "time", + "orientation": { + "type": "anatomical", + "value": "left-to-right", + }, + }, + {"name": "y", "type": "space", "unit": "micrometer"}, + {"name": "x", "type": "space", "unit": "micrometer"}, + ], + "datasets": [ + { + "path": "0", + "coordinateTransformations": [ + {"type": "scale", "scale": [1.0, 1.0, 1.0]} + ], + } + ], + } + ] + + with pytest.raises(ValueError, match="non-space axes"): + from_ngff_zarr(store, validate=True)