Data-flow audit minor findings: unknown scale, manifest flags, per-file roles, uint64 labels

AGENTS.md

@@ -301,8 +301,9 @@ functions the tests use.
 
 - **Manifest contents:**
   - settings, the confirmed labels per leaf, and the axis mapping;
-  - per-file size, `mtime_ns`, axes, `singleton_roles`, plane count and `position_id`
-    (optional key, null when absent; manifest version stays 1);
+  - per-file size, `mtime_ns`, axes, `singleton_roles`, plane count, `position_id` and
+    `roles` (the file's own axis roles; optional keys, null when absent; manifest version
+    stays 1); flags are JSON booleans only and the versions are strings (`"1"`, `"4"`);
   - channel names and significant bits (OME only, information only);
   - the CSV's SHA-256 and package versions (including `morphobricks-normalization`).
 - **A file is unchanged** when its size, mtime, labels and axis layout match and the table

src/morphobricks_preprocessing/cli.py

@@ -405,7 +405,7 @@ def progress(done: int, total: int, path: str) -> None:
     print()
     try:
         report = scan_masks(pre.masks, pre.discovery.root, progress=progress, thresholds=thresholds)
-    except ValueError as exc:  # masks whose pixels cannot be used (planes, negative labels)
+    except ValueError as exc:  # masks whose pixels cannot be used (planes, labels)
         print(f"The masks cannot be used: {exc}")
         return 1
     output = Path(args.output) if args.output else default_output_folder(pre.discovery.root)

src/morphobricks_preprocessing/manifest.py

@@ -126,6 +126,12 @@ class FileEntry:
         The image's position identifier, its name without the extension
         (:func:`~morphobricks_preprocessing.discovery.position_id`); None in
         manifests written before it was recorded.
+    roles
+        The role of each of the file's axes (``"X"``, ``"Y"``, ``"Z"``, ``"C"``,
+        ``"T"`` or None), from its own mapping: for an OME-Zarr image whose
+        declared order differs from the experiment mapping's, these are its
+        roles, not the experiment's. None in manifests written before they were
+        recorded.
     """
 
     relative_path: str
@@ -141,6 +147,7 @@ class FileEntry:
     read_in_blocks: bool = False
     position_id: str | None = None
     channel_names_trimmed: bool = False
+    roles: tuple[str | None, ...] | None = None
 
     def same_source(self, other: FileEntry) -> bool:
         """Return True if ``other`` describes the same unchanged file, read the same way."""
@@ -298,9 +305,11 @@ def to_dict(self) -> dict:
             "settings": {
                 "bin_count": self.settings.bin_count,
                 "memory_limit": self.settings.memory_limit,
-                "missing_as_singleton": self.settings.missing_as_singleton,
+                "missing_as_singleton": _flag(
+                    self.settings.missing_as_singleton, "missing_as_singleton"
+                ),
                 "exclude": list(self.settings.exclude),
-                "convert_tiffs": self.settings.convert_tiffs,
+                "convert_tiffs": _flag(self.settings.convert_tiffs, "convert_tiffs"),
                 "conversion": self.settings.conversion.to_dict(),
                 "histogram": self.settings.histogram.to_dict(),
             },
@@ -318,7 +327,11 @@ def to_dict(self) -> dict:
             "csv_sha256": self.csv_sha256,
             "experiment_design_sha256": self.experiment_design_sha256,
             "image_type": self.image_type,
-            "channels_from_rgb_samples": self.channels_from_rgb_samples,
+            "channels_from_rgb_samples": (
+                None
+                if self.channels_from_rgb_samples is None
+                else _flag(self.channels_from_rgb_samples, "channels_from_rgb_samples")
+            ),
             "intensity_checks": (
                 None if self.intensity_checks is None else self.intensity_checks.to_dict()
             ),
@@ -352,9 +365,9 @@ def from_dict(cls, data: dict) -> RunManifest:
                 settings=RunSettings(
                     bin_count=int(settings["bin_count"]),
                     memory_limit=int(settings["memory_limit"]),
-                    missing_as_singleton=bool(settings["missing_as_singleton"]),
+                    missing_as_singleton=_flag(settings["missing_as_singleton"]),
                     exclude=tuple(str(p) for p in settings["exclude"]),
-                    convert_tiffs=bool(settings.get("convert_tiffs", False)),
+                    convert_tiffs=_flag(settings.get("convert_tiffs", False)),
                     conversion=ConversionSettings.from_dict(settings.get("conversion")),
                     histogram=(
                         HistogramSettings()
@@ -377,7 +390,7 @@ def from_dict(cls, data: dict) -> RunManifest:
                 channels_from_rgb_samples=(
                     None
                     if data.get("channels_from_rgb_samples") is None
-                    else bool(data["channels_from_rgb_samples"])
+                    else _flag(data["channels_from_rgb_samples"])
                 ),
                 intensity_checks=(
                     None
@@ -425,6 +438,7 @@ def file_entry_for(
         read_in_blocks=read_in_blocks,
         position_id=position_id(relative_path),
         channel_names_trimmed=channel_names_trimmed,
+        roles=tuple(mapping.roles),
     )
 
 
@@ -528,9 +542,10 @@ def _file_dict(entry: FileEntry) -> dict:
         "channel_names": list(entry.channel_names),
         "significant_bits": entry.significant_bits,
         "ome_zarr_path": entry.ome_zarr_path,
-        "read_in_blocks": entry.read_in_blocks,
+        "read_in_blocks": _flag(entry.read_in_blocks, "read_in_blocks"),
         "position_id": entry.position_id,
         "channel_names_trimmed": entry.channel_names_trimmed,
+        "roles": None if entry.roles is None else list(entry.roles),
     }
 
 
@@ -551,12 +566,36 @@ def _file_entry(data: dict) -> FileEntry:
         channel_names=tuple(None if n is None else str(n) for n in data.get("channel_names", [])),
         significant_bits=None if bits is None else int(bits),
         ome_zarr_path=None if data.get("ome_zarr_path") is None else str(data["ome_zarr_path"]),
-        read_in_blocks=bool(data.get("read_in_blocks", False)),
+        read_in_blocks=_flag(data.get("read_in_blocks", False)),
         position_id=None if data.get("position_id") is None else str(data["position_id"]),
         channel_names_trimmed=bool(data.get("channel_names_trimmed", False)),
+        roles=_roles(data.get("roles"), str(data["axes"])),
     )
 
 
+def _roles(value: object, axes: str) -> tuple[str | None, ...] | None:
+    """Check a file's recorded axis roles: one role or null per axis (absent in older manifests)."""
+    if value is None:
+        return None
+    if not isinstance(value, list) or len(value) != len(axes):
+        raise ValueError(f"the roles {value!r} do not fit the axes {axes!r}")
+    roles = tuple(None if r is None else str(r) for r in value)
+    AxisMapping(axes, roles)  # raises ValueError for an unknown role
+    return roles
+
+
+def _flag(value: object, key: str | None = None) -> bool:
+    """Return a boolean; anything else (``"false"``, 0, null) is malformed, not guessed.
+
+    Used both when reading and when writing a manifest, so a manifest is never written
+    with a flag its reader would refuse (or that ``bool()`` would turn around).
+    """
+    if not isinstance(value, bool):
+        where = f" for {key}" if key else ""
+        raise ValueError(f"expected true or false{where}, found {value!r}")
+    return value
+
+
 def _plain(value):
     """Return a range check value as JSON data: Python numbers, lists for tuples."""
     if isinstance(value, tuple):

src/morphobricks_preprocessing/masks.py

@@ -234,7 +234,8 @@ def read_mask(
     ValueError
         If the mask's dimensions are not the ones its header gave (``mask.axes``), or ``z``
         or ``t`` is missing or outside the mask, or the plane holds negative labels
-        (label 0 is the background and objects are positive, contract §15).
+        (label 0 is the background and objects are positive, contract §15) or a label
+        above 2**63 - 1, which later bricks cannot store as an int64 ``label_id``.
     """
     wanted = {"Z": z, "T": t}
     for axis, index in wanted.items():
@@ -260,9 +261,17 @@ def read_mask(
     if axes != "YX" or (mask.axes and pixels.shape != tuple(_yx(mask.axes, mask.shape))):
         raise ValueError(f"the mask {mask.path} is not the 2D plane its header gave ({axes})")
     check_labels(pixels, mask.path)
+    if pixels.dtype == np.uint64 and pixels.size and pixels.max() > _LARGEST_LABEL:
+        raise ValueError(
+            f"the mask {mask.path} holds labels above {_LARGEST_LABEL} (largest "
+            f"{pixels.max()}), which cannot be stored as a 64-bit signed label number."
+        )
     return pixels
 
 
+_LARGEST_LABEL = np.iinfo(np.int64).max
+
+
 def _read_tiff_plane(path: Path, wanted: Mapping[str, int | None]) -> tuple[np.ndarray, str]:
     """Read the plane at ``wanted`` Z and T of a TIFF mask; return it and its axes.
 

src/morphobricks_preprocessing/physical_sizes.py

@@ -30,6 +30,10 @@
 
 logger = logging.getLogger(__name__)
 
+UNKNOWN_SCALE_KEY = "unknown_scale"
+"""Key, in an OME-NGFF multiscales entry's ``metadata``, listing the axes (``"x"``, ...)
+whose ``scale`` is a placeholder 1 because their size is not known (this brick's copies)."""
+
 # Units in OME and ImageJ metadata, and their OME-NGFF (UDUNITS-2) names.
 _SPACE_UNITS = {
     "pm": "picometer",
@@ -131,10 +135,15 @@ def __hash__(self) -> int:
         return hash(self._key())
 
     def scale(self, role: str) -> float:
-        """Return the size of one step along ``role`` (1 when unknown)."""
+        """Return the size of one step along ``role`` (1 when unknown, see :meth:`known`)."""
         value = {"X": self.x, "Y": self.y, "Z": self.z, "T": self.t}.get(role)
         return 1.0 if value is None or not value > 0 else float(value)
 
+    def known(self, role: str) -> bool:
+        """Return True if the size of one step along ``role`` (X, Y, Z or T) is known."""
+        value = {"X": self.x, "Y": self.y, "Z": self.z, "T": self.t}.get(role)
+        return value is not None and value > 0
+
     def unit(self, role: str) -> str | None:
         """Return the unit of ``role``, or None when unknown or for channels."""
         if role == "T":
@@ -222,8 +231,9 @@ def ngff_physical_sizes(multiscale: dict, axes: str) -> PhysicalSizes:
     stored in a separate file through ``path``) or with a malformed one, every
     size is None; an axis without a ``unit`` has the unit None, and a scale that
     is not a positive finite number is None. A scale given without a unit is
-    kept: it may be a placeholder (some writers, this brick's OME-Zarr copies
-    included, write 1 when the size is not known).
+    kept: it may be a placeholder (some writers write 1 when the size is not
+    known). This brick's OME-Zarr copies name such axes in the multiscales
+    entry's ``metadata`` (``"unknown_scale": ["x", "y"]``); their sizes are None.
 
     Parameters
     ----------
@@ -256,8 +266,13 @@ def ngff_physical_sizes(multiscale: dict, axes: str) -> PhysicalSizes:
     units: list[str | None] = [None] * len(axes)
     if isinstance(declared, list) and len(declared) == len(axes):
         units = [axis.get("unit") if isinstance(axis, dict) else None for axis in declared]
+    meta = multiscale.get("metadata")
+    unknown = meta.get(UNKNOWN_SCALE_KEY) if isinstance(meta, dict) else None
+    unknown = {str(a).upper() for a in unknown} if isinstance(unknown, list) else set()
     found: dict[str, tuple[float | None, str | None]] = {}
     for letter, size, unit in zip(axes, scale, units, strict=True):
+        if letter in unknown:
+            size = None
         if letter in "XYZT":
             table = _TIME_UNITS if letter == "T" else _SPACE_UNITS
             found.setdefault(letter, (_number(size), _ngff_unit(unit, table)))

src/morphobricks_preprocessing/run.py

@@ -427,10 +427,19 @@ def run_experiment(
     Raises
     ------
     ValueError
-        For an image type other than ``"fluorescence"`` and ``"rgb"``.
+        For an image type other than ``"fluorescence"`` and ``"rgb"``, or for
+        ``missing_as_singleton`` or ``convert_tiffs`` that is not True or False
+        (the manifest records them as JSON booleans, contract §9).
     """
     if image_type is not None:
         check_image_type(image_type)
+    for name, value in (
+        ("missing_as_singleton", missing_as_singleton),
+        ("convert_tiffs", convert_tiffs),
+    ):
+        # Refused before anything is read rather than when the manifest is written.
+        if not isinstance(value, bool):
+            raise ValueError(f"{name} must be True or False, found {value!r}")
     root = Path(experiment_root).resolve()
     output = default_output_folder(root) if output_folder is None else Path(output_folder).resolve()
     pre = preflight(

src/morphobricks_preprocessing/zarr_export.py

@@ -35,6 +35,7 @@
 
 from .axes import AxisMapping
 from .physical_sizes import (  # noqa: F401 - moved there, still importable from here
+    UNKNOWN_SCALE_KEY,
     PhysicalSizes,
     read_physical_sizes,
 )
@@ -435,6 +436,11 @@ def attributes(self) -> dict:
             "type": "mean",
             "metadata": {"description": "2 x 2 block mean; level 0 holds the source values"},
         }
+        # OME-NGFF 0.4 requires a scale for every axis, so an unknown size is written as 1;
+        # say which, so that 1 is never read as a real size (issue #39).
+        unknown = _unknown_scale(self.roles, self.sizes)
+        if unknown:
+            multiscale["metadata"][UNKNOWN_SCALE_KEY] = unknown
         if self.name:
             multiscale["name"] = self.name
         attrs = {"multiscales": [multiscale]}
@@ -507,11 +513,18 @@ def is_copy_path(output_folder: str | os.PathLike, relative_path: str) -> bool:
     return base in target.parents and target.name.endswith(OME_ZARR_SUFFIX)
 
 
+def _unknown_scale(roles: Iterable[str], sizes: PhysicalSizes) -> list[str]:
+    """Return the axes (``"x"``, ...) whose size is not known, so their scale 1 is a placeholder."""
+    return [role.lower() for role in roles if role in "XYZT" and not sizes.known(role)]
+
+
 def copy_units_current(copy: str | os.PathLike, sizes: PhysicalSizes) -> bool:
     """Return True if a copy's axis units are those its source's sizes give now.
 
     Copies written before each axis had its own unit (plan §12 question 24) may
-    carry X's unit on Y or Z; such copies are written again on the next run. Only
+    carry X's unit on Y or Z, and copies written before the unknown sizes were marked
+    (``unknown_scale``, issue #39) read as size 1; such copies are written again on the next
+    run. Only
     the copy's metadata file is read. A copy whose metadata cannot be read counts
     as out of date, so it is written again rather than trusted.
 
@@ -525,7 +538,11 @@ def copy_units_current(copy: str | os.PathLike, sizes: PhysicalSizes) -> bool:
     try:
         attrs = json.loads((Path(copy) / ".zattrs").read_text(encoding="utf-8"))
         axes = attrs["multiscales"][0]["axes"]
-        return all(axis.get("unit") == sizes.unit(str(axis["name"]).upper()) for axis in axes)
+        roles = [str(axis["name"]).upper() for axis in axes]
+        marked = attrs["multiscales"][0].get("metadata", {}).get(UNKNOWN_SCALE_KEY, [])
+        return marked == _unknown_scale(roles, sizes) and all(
+            axis.get("unit") == sizes.unit(role) for axis, role in zip(axes, roles, strict=True)
+        )
     except (OSError, ValueError, KeyError, IndexError, TypeError, AttributeError):
         logger.debug("Cannot read the axis units of the copy %s", copy, exc_info=True)
         return False

tests/test_manifest.py

@@ -1,7 +1,9 @@
 import hashlib
 import json
 import os
+from dataclasses import replace
 
+import numpy as np
 import pytest
 
 from morphobricks_preprocessing import (
@@ -245,3 +247,76 @@ def test_bad_role_in_manifest_is_refused():
     data["axis_mapping"]["roles"] = ["W", "Y", "X"]
     with pytest.raises(ManifestFormatError):
         RunManifest.from_dict(data)
+
+
+# Data-flow audit, issue #39 -----------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    ("where", "key"),
+    [
+        ("settings", "missing_as_singleton"),
+        ("settings", "convert_tiffs"),
+        ("file", "read_in_blocks"),
+        ("top", "channels_from_rgb_samples"),
+    ],
+)
+@pytest.mark.parametrize("value", ["false", "true", 0, 1])
+def test_flags_must_be_json_booleans(where, key, value):
+    data = manifest(channels_from_rgb_samples=False).to_dict()
+    target = {"settings": data["settings"], "file": data["files"][0], "top": data}[where]
+    target[key] = value  # "false" was read as True with bool()
+    with pytest.raises(ManifestFormatError, match="expected true or false"):
+        RunManifest.from_dict(data)
+
+
+def _with_flag(key, value):
+    if key == "read_in_blocks":
+        return manifest(files=(entry(read_in_blocks=value),))
+    if key == "channels_from_rgb_samples":
+        return manifest(channels_from_rgb_samples=value)
+    return manifest(settings=replace(RunSettings(65_536, 2**30, False, ()), **{key: value}))
+
+
+@pytest.mark.parametrize(
+    "key", ["missing_as_singleton", "convert_tiffs", "read_in_blocks", "channels_from_rgb_samples"]
+)
+@pytest.mark.parametrize("value", ["false", 0, 1, np.bool_(True)])
+def test_the_writer_refuses_flags_that_are_not_booleans(tmp_path, key, value):
+    # Review of #44: the writer is as strict as the reader. It used to write "false" or 0
+    # as given, leaving a manifest the reader refuses (or that bool() turned around).
+    path = tmp_path / "m.json"
+    with pytest.raises(ValueError, match=f"expected true or false for {key}"):
+        write_manifest(_with_flag(key, value), path)
+    assert not path.exists()
+
+
+def test_the_writer_keeps_null_for_an_unknown_rgb_flag(tmp_path):
+    path = write_manifest(manifest(channels_from_rgb_samples=None), tmp_path / "m.json")
+    assert json.loads(path.read_bytes())["channels_from_rgb_samples"] is None
+
+
+def test_each_files_axis_roles_round_trip_and_are_none_when_absent(tmp_path):
+    original = manifest(
+        files=(
+            entry(roles=("Z", "Y", "X")),
+            entry("cond/rep/b.zarr", axes="CYX", roles=(None,) * 3),
+        )
+    )
+    path = write_manifest(original, tmp_path / "m.json")
+    assert read_manifest(path) == original
+    data = json.loads(path.read_bytes())
+    assert [f["roles"] for f in data["files"]] == [["Z", "Y", "X"], [None, None, None]]
+    del data["files"][0]["roles"]  # written before the roles were recorded
+    path.write_text(json.dumps(data), encoding="utf-8")
+    assert read_manifest(path).files[0].roles is None
+    data["files"][1]["roles"] = ["Z", "Y"]  # not one role per axis
+    path.write_text(json.dumps(data), encoding="utf-8")
+    with pytest.raises(ManifestFormatError):
+        read_manifest(path)
+
+
+def test_file_entry_for_records_the_files_own_roles(tmp_path):
+    write_tiff(tmp_path / "a.tif")
+    mapping = prefill_mapping("ZYX").with_role(0, None)
+    assert file_entry_for(tmp_path, "a.tif", ("c", "b", "t"), mapping).roles == (None, "Y", "X")