From 699e5b46b7bfee3de5192fc59549ce4ae0828a9a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 20:24:43 +0000 Subject: [PATCH 1/3] Fix data-flow audit minor findings: unknown scale, manifest flags, file roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - OME-Zarr copies: OME-NGFF 0.4 needs a scale per axis, so an unknown pixel size is still written as 1, but the axes are now listed in the multiscales metadata as "unknown_scale" and ngff_physical_sizes gives None for them. Copies without the list are converted again (rows unchanged). - Manifest flags (missing_as_singleton, convert_tiffs, read_in_blocks, channels_from_rgb_samples) must be JSON booleans; "false" was read as True. - Each file entry records its own axis roles ("roles", optional key), so consumers need not rebuild them from the experiment mapping (wrong for OME-Zarr images in another order). - read_mask refuses uint64 labels above 2**63 - 1 (no int64 label_id); the masks command reports it and exits 1. - Contract §9 says manifest_version/schema_version are strings. The CSV-column check (plane_id, image_file, bit_depth) belongs to the normalization brick's reader and is not changed here. Part of #39 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Scma6W3LC11s3qjXHZuGDm --- AGENTS.md | 5 +- docs/contract.md | 11 +++-- src/morphobricks_preprocessing/cli.py | 6 ++- src/morphobricks_preprocessing/manifest.py | 36 ++++++++++++-- src/morphobricks_preprocessing/masks.py | 11 ++++- .../physical_sizes.py | 21 +++++++-- src/morphobricks_preprocessing/zarr_export.py | 21 ++++++++- tests/test_manifest.py | 47 +++++++++++++++++++ tests/test_masks.py | 22 +++++++++ tests/test_zarr_export.py | 39 +++++++++++++++ tests/test_zarr_input.py | 12 +++++ 11 files changed, 213 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e313882a..deea89b7 100644 --- a/AGENTS.md +++ b/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 diff --git a/docs/contract.md b/docs/contract.md index f90fe6e7..f3a4cdfe 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -53,7 +53,7 @@ channel order; an image without a C axis has one channel. | OME-Zarr labels | The label images of an image's OME-NGFF `labels` group (0.4 and 0.5) are listed with `ngff_label_names` and opened, level 0, with `open_ngff_labels` (by name; the name may be left out when there is one). They are never analysed; later bricks such as patch extraction use them as masks. | | Experiment mapping | Defined on the TIFF arrangement when the experiment has TIFF files; in an experiment of OME-Zarr images only, on their arrangement with the most axes. | | Channel names, significant bits | Recorded in the run manifest when metadata provides them; not part of the CSV. Channel names that differ at the same channel index are a warning. | -| Physical sizes | Read with the header, not part of the CSV or the manifest, and compared between images in the folder check (§11, in micrometres with each axis's own unit; a size without a known unit cannot be compared): `PhysicalSizes` with the size of one step along X, Y, Z and T and each axis's unit (OME-NGFF unit names). TIFF: OME `PhysicalSize{X,Y,Z}` with their own units (micrometres when left out) and `TimeIncrement`, or ImageJ resolution, `spacing` and `finterval` with `unit`, `yunit`, `zunit`. OME-Zarr (0.4 and 0.5): the `scale` of level 0's `coordinateTransformations`, times the multiscales entry's own `scale` when given, with each axis's `unit` (common spellings such as `µm`, `um`, `micron` and `s` given as UDUNITS-2 names, others as written). Ångström (`Å`) and ImageJ's escaped micro sign (`\u00B5m`) are recognised. Nothing is guessed: what the metadata does not give is None; a scale in a separate file (`path`) or not one number per axis gives no sizes; an OME-Zarr scale without a unit is kept with the unit None. | +| Physical sizes | Read with the header, not part of the CSV or the manifest, and compared between images in the folder check (§11, in micrometres with each axis's own unit; a size without a known unit cannot be compared): `PhysicalSizes` with the size of one step along X, Y, Z and T and each axis's unit (OME-NGFF unit names). TIFF: OME `PhysicalSize{X,Y,Z}` with their own units (micrometres when left out) and `TimeIncrement`, or ImageJ resolution, `spacing` and `finterval` with `unit`, `yunit`, `zunit`. OME-Zarr (0.4 and 0.5): the `scale` of level 0's `coordinateTransformations`, times the multiscales entry's own `scale` when given, with each axis's `unit` (common spellings such as `µm`, `um`, `micron` and `s` given as UDUNITS-2 names, others as written). Ångström (`Å`) and ImageJ's escaped micro sign (`\u00B5m`) are recognised. Nothing is guessed: what the metadata does not give is None; a scale in a separate file (`path`) or not one number per axis gives no sizes; an OME-Zarr scale without a unit is kept with the unit None, except on the axes a multiscales entry's `metadata` lists as `unknown_scale` (this brick's copies, §10), whose sizes are None. | | Plane counts | Images may have different numbers of depth slices, time points or planes along an unassigned dimension; this is a warning (§11). | | Channel counts | Every image of an experiment has the same number of channels (normalization plan N43): along the C axis, 1 without one (also when a missing C axis is confirmed as length 1). A different number is the error `different_channel_counts`, found before any image is read: in the folder check from the headers (the `C` dimension, or the RGB samples `S` when there is no `C`), and again with the confirmed axis roles, which decide (§11). | @@ -151,14 +151,14 @@ file's schema or statistical interpretation. | Key | Meaning | |---|---| -| `manifest_version`, `schema_version` | Manifest format (`1`) and CSV format (`4`). A manifest whose `schema_version` is not the current one makes the next run recompute every row. | +| `manifest_version`, `schema_version` | Manifest format (`"1"`) and CSV format (`"4"`), written as JSON **strings**, not numbers. A manifest whose `schema_version` is not the current one makes the next run recompute every row. | | `written_at` | UTC time of writing, ISO 8601. | | `experiment_root` | Absolute path of the experiment folder, for information. | | `dtype` | Pixel type of the experiment. | | `settings` | `bin_count`, `memory_limit` (bytes), `missing_as_singleton`, `exclude` (sorted relative paths), `convert_tiffs` (whether TIFF images are converted to OME-Zarr and processed from the copies; false when absent), `conversion` (`chunk_size`, `compression`, `compression_level`, `pyramid_levels` — null for automatic; defaults when absent), `histogram` (`HistogramSettings.to_dict()` of the normalization brick: `library_version`, `histogram_range`, `robust_quantiles`, `min_central_bins`, §4; the default `minmax` range when absent, in manifests written before follow-up P6). | | `axis_mapping` | `axes` (file axis labels) and `roles` (`X`, `Y`, `Z`, `C`, `T` or null per axis). | | `labels` | One entry per leaf folder: `folder` (relative path, `.` for the root) and its three labels. | -| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count` (its rows in the table: XYC planes), `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent), `position_id` (the image's position identifier, §1; null when absent, in manifests written before it). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | +| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count` (its rows in the table: XYC planes), `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent), `position_id` (the image's position identifier, §1; null when absent, in manifests written before it), `roles` (the role of each of the file's `axes`, `X`, `Y`, `Z`, `C`, `T` or null, from the file's own mapping: an OME-Zarr image in another order than `axis_mapping` has its own roles, so later bricks read these rather than rebuild them from `axis_mapping`; null when absent, in manifests written before them; optional key, version 1). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | | `csv_sha256` | SHA-256 of the table written with this manifest. | | `experiment_design_sha256` | SHA-256 of the `experiment_design.csv` written with this run (§12), so later bricks can check that the design they read is the one used; null when the run had no design, or absent in manifests written before it. Optional key: the manifest stays version 1. | | `masks` | The masks paired with the images (§15), when the run was given a masks folder: `folder` (absolute path of the masks folder, null when only OME-Zarr label images were used), `types` (sorted mask-type names) and `files` (one entry per image and mask type, sorted by image then type: `image`, `type`, `path`, `source` `folder` or `ome-zarr`). Absent when the run had no masks. Optional key: the manifest stays version 1, and the table does not depend on the masks. | @@ -166,6 +166,7 @@ file's schema or statistical interpretation. | `channels_from_rgb_samples` | `true` when the C role is the RGB samples axis of images stored as RGB colour (§13), `false` otherwise; absent in manifests written before it. Optional key. | | `intensity_checks` | The limits of the intensity checks run on the table (§14): `IntensityCheckSettings.to_dict()` of the normalization brick (`library_version`, `outlier_limit`, `saturation_limit`, `min_images`); absent in manifests written before it. Optional key. Changing the limits recomputes nothing. | | `narrow_histogram_ranges` | The channel planes whose range check found the central range too narrow (§4), sorted by plane and channel: `relative_path`, `plane_id`, `channel` and the fields of the normalization brick's `RangeCheck` (`histogram_range`, `central_percentiles`, `central_bins`, `robust_quantiles`, `robust_limits`, `outside_robust_count`, `robust_central_bins`, `min_central_bins`). Empty when there are none; absent in manifests written before follow-up P6. Optional key: the manifest stays version 1. | +| Flags | `missing_as_singleton`, `convert_tiffs`, `read_in_blocks` and `channels_from_rgb_samples` are JSON booleans (`true`/`false`); any other value (`"false"`, `0`) makes the manifest malformed, never read as true or false. | | `versions` | Versions of `morphobricks-preprocessing`, `morphobricks-normalization` (which computes the histograms and writes the CSV; absent in manifests written before it), `numpy`, `tifffile` and Python. Information only: a different version never causes a rebuild. | Channel names and significant bits come from OME metadata and are for information only. @@ -192,7 +193,7 @@ presentation does not change the conversion rules below. | Level 0 | The source values, unchanged, in the source pixel type; chunks of one plane along Z, C and T and at most the chunk size (16 to 8,192 px, default 1,024) in X and Y | | Further levels | 2 x 2 block means rounded to the pixel type; automatic (default: until a plane fits in 512 x 512, at most 8 levels in all), none, or 1 to 7 levels; for viewing only | | Large planes | A plane read in blocks is written block by block; its further levels are then built from the stored level above, in regions of even size, and equal those of a plane written whole | -| Scale | Physical pixel sizes, z-step and time step from OME or ImageJ metadata, with OME-NGFF units, each axis with its own unit (OME `PhysicalSizeXUnit`, `PhysicalSizeYUnit`, `PhysicalSizeZUnit`, micrometres when left out; ImageJ `unit`, `yunit`, `zunit`); 1 when unknown | +| Scale | Physical pixel sizes, z-step and time step from OME or ImageJ metadata, with OME-NGFF units, each axis with its own unit (OME `PhysicalSizeXUnit`, `PhysicalSizeYUnit`, `PhysicalSizeZUnit`, micrometres when left out; ImageJ `unit`, `yunit`, `zunit`). OME-NGFF 0.4 requires a scale for every axis, so an unknown size is written as 1 without a unit and its axis is listed in the multiscales entry's `metadata` as `"unknown_scale": ["z", "y", "x"]` (axis names; absent when every size is known). `ngff_physical_sizes` gives None for those axes, so the placeholder is never read as a size; other readers must check the list. A copy written before the list (its list differs from the one its TIFF gives now) counts as changed and is converted again; its rows do not change | | Channels | `omero` block with channel names from metadata (or `Channel `) and each channel's observed minimum and maximum as display window | | Updates | Copies of unchanged images are kept, of new or changed images rewritten, and of images no longer analysed removed; a failed run leaves the earlier copies as they were (see §9 for publication) | | Switching | Turning conversion on or off recomputes every TIFF row; turning it off removes the copies the manifest recorded | @@ -308,7 +309,7 @@ masks/ masks folder, outside the experiment folder | Layout | Either one subfolder per mask type, each mirroring the experiment's folders (`//.../.tif`), or the experiment's folders with the type after the image name (`/.../_.tif`, as segmentation tools often write it, e.g. MorphoRelate's JUMP-Target-2 conversion); the two may be mixed. Any number of mask types. The label images of an OME-Zarr image (its OME-NGFF `labels` group, with the `zarr` extra) are masks too, of the type named by the label image; they are used whenever masks are asked for, with or without a masks folder. | | Pairing | A mask belongs to the image in the same leaf folder with the same `position_id` (§1), ignoring upper/lower case; its file is a TIFF (`.tif`, `.tiff`, optionally `.ome`, any case). Excluded images have no masks. | | Names | A mask type is named by its folder, by the suffix after the image name, or by its label image. A file that could belong to two images (an image name that starts another's, `a_b_cells.tif` beside images `a` and `a_b`) or fit both layouts is the error `ambiguous_mask`. Two names differing only in case are the error `mask_names_differ_in_case`; one image with two masks of one type (two extensions, or a file and a label image) is the error `duplicate_mask`. | -| Header checks | From the headers only, no pixels read (`check`, `run`, `preflight`): a mask is 2D once its length-1 axes are removed (`mask_not_2d`), has the Y and X sizes of its image (`mask_size_mismatch`), and integer or boolean pixels (`mask_pixel_type`); a mask that cannot be opened, or holds several series, is `unreadable_mask`. All four are errors. | +| Header checks | From the headers only, no pixels read (`check`, `run`, `preflight`): a mask is 2D once its length-1 axes are removed (`mask_not_2d`), has the Y and X sizes of its image (`mask_size_mismatch`), and integer or boolean pixels (`mask_pixel_type`; a `uint64` mask holding a label above 2^63 − 1, which later bricks cannot store as an int64 `label_id`, raises an error when its pixels are read); a mask that cannot be opened, or holds several series, is `unreadable_mask`. All four are errors. | | Folder | The masks folder must exist (`masks_folder_not_found`) and be outside the experiment folder (`masks_inside_experiment`), where its folders would be read as labels. Both errors. | | Warnings | `mask_missing` (images without a mask of a type that other images have: their objects of that type cannot be measured), `mask_without_image` (files matching no analysed image), `mask_outside_type_folder` (files directly in the masks folder that are not TIFF masks). | | Run | `run_experiment(..., masks_folder=...)` stops on a mask error and records the masks in the manifest's `masks` key (§9). Without masks, the run and its outputs are unchanged. | diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index aec77490..fe3196b8 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -403,7 +403,11 @@ def progress(done: int, total: int, path: str) -> None: print(f"[{done + 1}/{total}] {path}", flush=True) print() - report = scan_masks(pre.masks, pre.discovery.root, progress=progress, thresholds=thresholds) + try: + report = scan_masks(pre.masks, pre.discovery.root, progress=progress, thresholds=thresholds) + except ValueError as exc: # a mask whose pixels cannot be used + print(f"The masks cannot be used: {exc}") + return 1 output = Path(args.output) if args.output else default_output_folder(pre.discovery.root) path = write_mask_report(report, output / MASK_REPORT_FILENAME) print() diff --git a/src/morphobricks_preprocessing/manifest.py b/src/morphobricks_preprocessing/manifest.py index 273e60c6..229ce31e 100644 --- a/src/morphobricks_preprocessing/manifest.py +++ b/src/morphobricks_preprocessing/manifest.py @@ -123,6 +123,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 @@ -137,6 +143,7 @@ class FileEntry: ome_zarr_path: str | None = None read_in_blocks: bool = False position_id: str | None = None + 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.""" @@ -348,9 +355,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() @@ -373,7 +380,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 @@ -419,6 +426,7 @@ def file_entry_for( ome_zarr_path=ome_zarr_path, read_in_blocks=read_in_blocks, position_id=position_id(relative_path), + roles=tuple(mapping.roles), ) @@ -524,6 +532,7 @@ def _file_dict(entry: FileEntry) -> dict: "ome_zarr_path": entry.ome_zarr_path, "read_in_blocks": entry.read_in_blocks, "position_id": entry.position_id, + "roles": None if entry.roles is None else list(entry.roles), } @@ -544,11 +553,30 @@ 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"]), + 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) -> bool: + """Return a JSON boolean; anything else (``"false"``, 0, null) is malformed, not guessed.""" + if not isinstance(value, bool): + raise ValueError(f"expected true or false, 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): diff --git a/src/morphobricks_preprocessing/masks.py b/src/morphobricks_preprocessing/masks.py index 2b45abb7..c6994f9c 100644 --- a/src/morphobricks_preprocessing/masks.py +++ b/src/morphobricks_preprocessing/masks.py @@ -184,7 +184,8 @@ def read_mask(mask: MaskFile, masks_folder: Path | None, experiment_root: Path) Raises ------ ValueError - If the mask is not 2D once its length-1 axes are removed. + If the mask is not 2D once its length-1 axes are removed, or holds a label above + 2**63 - 1, which later bricks cannot store as an int64 ``label_id``. """ if mask.source == FOLDER: pixels = tifffile.imread(Path(masks_folder) / mask.path) @@ -195,9 +196,17 @@ def read_mask(mask: MaskFile, masks_folder: Path | None, experiment_root: Path) pixels = np.squeeze(pixels) if pixels.ndim != 2: raise ValueError(f"the mask {mask.path} is not 2D ({pixels.shape})") + 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 _folder_issues(folder: Path, experiment_root: Path) -> list[ValidationIssue]: if not folder.is_dir(): return [ diff --git a/src/morphobricks_preprocessing/physical_sizes.py b/src/morphobricks_preprocessing/physical_sizes.py index 71659080..96222c37 100644 --- a/src/morphobricks_preprocessing/physical_sizes.py +++ b/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))) diff --git a/src/morphobricks_preprocessing/zarr_export.py b/src/morphobricks_preprocessing/zarr_export.py index 933022b6..5d54a042 100644 --- a/src/morphobricks_preprocessing/zarr_export.py +++ b/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]} @@ -499,11 +505,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. @@ -517,7 +530,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 diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 7a55af19..0477120e 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -245,3 +245,50 @@ 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 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") diff --git a/tests/test_masks.py b/tests/test_masks.py index 508ea129..d3ba1b66 100644 --- a/tests/test_masks.py +++ b/tests/test_masks.py @@ -304,3 +304,25 @@ def test_masks_command_takes_thresholds(experiment, masks, tmp_path, capsys): assert report["thresholds"]["same_objects_iou"] == 0.8 assert main([*args, "--grouped-label-share", "1.5"]) == 1 assert "must be above 0 and at most 1" in capsys.readouterr().out + + +def test_uint64_labels_above_the_int64_range_are_refused(experiment, masks, tmp_path, capsys): + # Issue #39: later bricks store labels as int64 label_id. + from morphobricks_preprocessing import read_mask + + huge = label_image(0).astype(np.uint64) + huge[0, 0] = 2**63 + write_data(masks / "Cells" / "ctrl" / "rep1" / "a.tif", huge) + found = inventory(experiment, masks) + assert found.ok # headers only + cells = found.for_image("ctrl/rep1/a.tif")["Cells"] + with pytest.raises(ValueError, match="labels above 9223372036854775807"): + read_mask(cells, found.folder, experiment) + fits = huge.copy() + fits[0, 0] = 2**63 - 1 + write_data(masks / "Cells" / "ctrl" / "rep1" / "a.tif", fits) + assert read_mask(cells, found.folder, experiment)[0, 0] == 2**63 - 1 + write_data(masks / "Cells" / "ctrl" / "rep1" / "a.tif", huge) + out = tmp_path / "out" + assert main(["masks", str(experiment), "--masks", str(masks), "--output", str(out)]) == 1 + assert "cannot be stored as a 64-bit signed label number" in capsys.readouterr().out diff --git a/tests/test_zarr_export.py b/tests/test_zarr_export.py index 3dfa1e81..574e5007 100644 --- a/tests/test_zarr_export.py +++ b/tests/test_zarr_export.py @@ -888,3 +888,42 @@ def uneven_blocks(): tmp_path / "tiny.ome.zarr", mapping, layout, settings=settings, memory_limit=1 ) assert tiny._region_side() == 2 # the smallest region that halves exactly + + +@needs_zarr +def test_unknown_pixel_sizes_are_marked_not_read_as_1(tmp_path): + # Issue #39: OME-NGFF 0.4 needs a scale per axis, so an unknown size is written as 1; + # the copy says which axes those are, and reading the copy gives no size for them. + from morphobricks_preprocessing import open_ngff + + write_data(tmp_path / "exp" / "a.tif", random_data((2, 8, 8), "uint8"), axes="ZYX") + result = run_experiment(tmp_path / "exp", ZYX, convert_tiffs=True) + copy = result.output_folder / ome_zarr_relative_path("a.tif") + meta = multiscale(open_copy(result.output_folder, "a.tif")) + assert meta["datasets"][0]["coordinateTransformations"][0]["scale"] == [1.0, 1.0, 1.0] + assert meta["metadata"]["unknown_scale"] == ["z", "y", "x"] + assert open_ngff(copy).physical_sizes == PhysicalSizes() + # A copy written before the marker counts as out of date and is written again. + attrs_path = copy / ".zattrs" + attrs = json.loads(attrs_path.read_text(encoding="utf-8")) + del attrs["multiscales"][0]["metadata"]["unknown_scale"] + attrs_path.write_text(json.dumps(attrs), encoding="utf-8") + again = run_experiment(tmp_path / "exp", ZYX, convert_tiffs=True) + assert again.converted == ("a.tif",) + assert run_experiment(tmp_path / "exp", ZYX, convert_tiffs=True).converted == () + + +@needs_zarr +def test_known_pixel_sizes_are_not_marked(tmp_path): + path = tmp_path / "exp/a.ome.tif" + path.parent.mkdir(parents=True) + tifffile.imwrite( + path, + random_data((2, 8, 8), "uint8"), + ome=True, + photometric="minisblack", + metadata={"axes": "ZYX", "PhysicalSizeX": 0.5, "PhysicalSizeY": 0.5}, + ) + result = run_experiment(tmp_path / "exp", ZYX, convert_tiffs=True) + meta = multiscale(open_copy(result.output_folder, "a.ome.tif")) + assert meta["metadata"]["unknown_scale"] == ["z"] # no z-step in the metadata diff --git a/tests/test_zarr_input.py b/tests/test_zarr_input.py index 6f1048c3..d307ed9f 100644 --- a/tests/test_zarr_input.py +++ b/tests/test_zarr_input.py @@ -392,6 +392,18 @@ def test_mixed_formats_with_different_axis_orders_are_not_a_dimension_conflict(t assert report.issues == () +def test_the_manifest_records_each_files_own_axis_roles(tmp_path): + # Issue #39: an OME-Zarr image in another order than the experiment mapping has its own + # roles; a consumer must not rebuild them from the experiment mapping. + write_data(tmp_path / "exp" / "a.tif", random_data((3, 2, 8, 8)), axes="ZCYX", imagej=True) + write_ngff(tmp_path / "exp" / "b.ome.zarr", random_data((2, 3, 8, 8)), "czyx") + result = run_experiment(tmp_path / "exp", prefill_mapping("ZCYX"), output_folder=tmp_path / "o") + assert result.written, result.issues + manifest = read_manifest(result.manifest_path) + assert manifest.file("a.tif").roles == ("Z", "C", "Y", "X") + assert manifest.file("b.ome.zarr").roles == ("C", "Z", "Y", "X") + + # Axes ------------------------------------------------------------------------------------ From 44b101b2514db1319a52f17268c633528487e726 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 06:35:12 +0000 Subject: [PATCH 2/3] Make the manifest writer strict about flags too Review of #44 (issue #39): the reader refuses flags that are not JSON booleans, but the writer wrote whatever value it was given ("false", 0, 1), producing a manifest its own reader refuses. write_manifest now checks missing_as_singleton, convert_tiffs, read_in_blocks and channels_from_rgb_samples (null allowed) with the same rule and raises ValueError naming the key, before anything is written. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VbytvBRB9CmKQ5AAkhEFcv --- docs/contract.md | 2 +- src/morphobricks_preprocessing/manifest.py | 25 +++++++++++++------ tests/test_manifest.py | 28 ++++++++++++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/docs/contract.md b/docs/contract.md index f3a4cdfe..2948e42c 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -166,7 +166,7 @@ file's schema or statistical interpretation. | `channels_from_rgb_samples` | `true` when the C role is the RGB samples axis of images stored as RGB colour (§13), `false` otherwise; absent in manifests written before it. Optional key. | | `intensity_checks` | The limits of the intensity checks run on the table (§14): `IntensityCheckSettings.to_dict()` of the normalization brick (`library_version`, `outlier_limit`, `saturation_limit`, `min_images`); absent in manifests written before it. Optional key. Changing the limits recomputes nothing. | | `narrow_histogram_ranges` | The channel planes whose range check found the central range too narrow (§4), sorted by plane and channel: `relative_path`, `plane_id`, `channel` and the fields of the normalization brick's `RangeCheck` (`histogram_range`, `central_percentiles`, `central_bins`, `robust_quantiles`, `robust_limits`, `outside_robust_count`, `robust_central_bins`, `min_central_bins`). Empty when there are none; absent in manifests written before follow-up P6. Optional key: the manifest stays version 1. | -| Flags | `missing_as_singleton`, `convert_tiffs`, `read_in_blocks` and `channels_from_rgb_samples` are JSON booleans (`true`/`false`); any other value (`"false"`, `0`) makes the manifest malformed, never read as true or false. | +| Flags | `missing_as_singleton`, `convert_tiffs`, `read_in_blocks` and `channels_from_rgb_samples` are JSON booleans (`true`/`false`); any other value (`"false"`, `0`) makes the manifest malformed, never read as true or false, and `write_manifest` refuses to write one (`ValueError`), so a written manifest always reads back. | | `versions` | Versions of `morphobricks-preprocessing`, `morphobricks-normalization` (which computes the histograms and writes the CSV; absent in manifests written before it), `numpy`, `tifffile` and Python. Information only: a different version never causes a rebuild. | Channel names and significant bits come from OME metadata and are for information only. diff --git a/src/morphobricks_preprocessing/manifest.py b/src/morphobricks_preprocessing/manifest.py index 229ce31e..0aec8667 100644 --- a/src/morphobricks_preprocessing/manifest.py +++ b/src/morphobricks_preprocessing/manifest.py @@ -301,9 +301,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(), }, @@ -321,7 +323,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() ), @@ -530,7 +536,7 @@ 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, "roles": None if entry.roles is None else list(entry.roles), } @@ -570,10 +576,15 @@ def _roles(value: object, axes: str) -> tuple[str | None, ...] | None: return roles -def _flag(value: object) -> bool: - """Return a JSON boolean; anything else (``"false"``, 0, null) is malformed, not guessed.""" +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): - raise ValueError(f"expected true or false, found {value!r}") + where = f" for {key}" if key else "" + raise ValueError(f"expected true or false{where}, found {value!r}") return value diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 0477120e..2445a950 100644 --- a/tests/test_manifest.py +++ b/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 ( @@ -268,6 +270,32 @@ def test_flags_must_be_json_booleans(where, key, value): 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=( From 91282500206f25b4bfd8f76bb6123c7c038b8c1d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 06:43:45 +0000 Subject: [PATCH 3/3] Refuse non-boolean run flags before reading any image Review of #44 (issue #39): write_manifest now refuses flags that are not booleans, so run_experiment(missing_as_singleton=1) or convert_tiffs="false" read every image (with the truthy value) and then failed when writing the manifest. run_experiment refuses both flags up front with a ValueError naming the flag. Regression test test_run_refuses_flags_that_are_not_booleans_before_reading. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VbytvBRB9CmKQ5AAkhEFcv --- src/morphobricks_preprocessing/run.py | 11 ++++++++++- tests/test_run.py | 11 +++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/morphobricks_preprocessing/run.py b/src/morphobricks_preprocessing/run.py index 4c7d7d68..297eff1c 100644 --- a/src/morphobricks_preprocessing/run.py +++ b/src/morphobricks_preprocessing/run.py @@ -424,10 +424,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( diff --git a/tests/test_run.py b/tests/test_run.py index 664906b7..55d39300 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -879,3 +879,14 @@ def test_sample_planes_report_problems(tmp_path): assert misfit.relative_path == "b.tif" and "does not fit" in misfit.error too_big = run_module.sample_planes(pre, ZYX, memory_limit=100) assert "limit" in too_big[0].error and too_big[0].plane is None + + +@pytest.mark.parametrize("flag", ["missing_as_singleton", "convert_tiffs"]) +@pytest.mark.parametrize("value", [1, "false", np.bool_(True)]) +def test_run_refuses_flags_that_are_not_booleans_before_reading(experiment, tmp_path, flag, value): + # Review of #44: the manifest writer refuses such flags; the run refuses them up + # front instead of after every image has been read. + output = tmp_path / "out" + with pytest.raises(ValueError, match=f"{flag} must be True or False"): + run_experiment(experiment, ZYX, output_folder=output, **{flag: value}) + assert not output.exists()