Skip to content

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

Merged
guijacquemet merged 4 commits into
mainfrom
claude/audit-minor-findings
Oct 30, 2026
Merged

guijacquemet merged 4 commits into
mainfrom
claude/audit-minor-findings

Conversation

@guijacquemet

Copy link
Copy Markdown
Contributor

Summary

Part of #39. Five of its six checklist items are fixed here; the CSV-column item is left for the normalization brick (see below).

  • Unknown pixel size written as scale 1 (zarr_export.py, physical_sizes.py). OME-NGFF 0.4 requires a scale on every axis, so the transform stays and 1 is still written. What changes:
    • The multiscales entry's metadata now lists those axes as "unknown_scale": ["z", "y", "x"].
    • ngff_physical_sizes returns None for them, so 1 is never read as a size.
    • PhysicalSizes.known(role) is added.
    • copy_units_current also compares this list, so copies written before it are converted again. Their rows do not change.
  • Manifest flags read with bool() (manifest.py). missing_as_singleton, convert_tiffs, read_in_blocks and channels_from_rgb_samples must now be JSON booleans. Any other value ("false", 0) raises ManifestFormatError, and the run then rebuilds as for any unreadable manifest. Before, "false" was read as True.
  • Per-file axis roles (manifest.py). FileEntry.roles and the file entry key roles hold one role or null per axis, from the file's own mapping. This is correct for OME-Zarr images whose declared order differs from axis_mapping. It is an optional key, so the manifest stays version 1; it is null in older manifests and filled on the next run, which writes fresh entries.
  • uint64 labels above 2^63 (masks.py, cli.py). read_mask raises ValueError for such labels. The masks command prints the message and exits 1.
  • Version types in the contract. Contract §9 now says manifest_version and schema_version are JSON strings ("1", "4").
  • Docs: contract §2, §9 (new Flags row, roles), §10 (Scale row) and §15; the AGENTS.md manifest summary.

Not done: CSV columns ignored

The plane_id, image_file and bit_depth columns are ignored and plane_id recomputed by morphobricks-normalization's read_plane_histograms (v0.3.1). That reader lives in that repository, which I was not to modify here. It should be fixed there, and this brick can then pin the new tag. Within this brick, reused rows are already guarded by the manifest's csv_sha256. So this PR does not close #39.

Notes for review

Tests

New tests:

  • tests/test_manifest.py::test_flags_must_be_json_booleans (16 cases)
  • tests/test_manifest.py::test_each_files_axis_roles_round_trip_and_are_none_when_absent
  • tests/test_manifest.py::test_file_entry_for_records_the_files_own_roles
  • tests/test_zarr_input.py::test_the_manifest_records_each_files_own_axis_roles (TIFF ZCYX next to OME-Zarr czyx)
  • tests/test_zarr_export.py::test_unknown_pixel_sizes_are_marked_not_read_as_1
  • tests/test_zarr_export.py::test_known_pixel_sizes_are_not_marked
  • tests/test_masks.py::test_uint64_labels_above_the_int64_range_are_refused

Results:

  • All 22 new test cases fail on main.
  • Full suite (Python 3.13, zarr 3.4.2, notebook extra installed): 1073 passed, 1 skipped. The skip is test_discovery.py:472: running as root ignores file modes.
  • ruff check . and ruff format --check . are clean.
  • Not run: zarr 2.18 / Python 3.11, Windows, macOS.

Generated by Claude Code

…le roles

- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Scma6W3LC11s3qjXHZuGDm
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VbytvBRB9CmKQ5AAkhEFcv

Copy link
Copy Markdown
Contributor Author

Review follow-up, pushed as 44b101b:


Generated by Claude Code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VbytvBRB9CmKQ5AAkhEFcv
…ndings

# Conflicts:
#	docs/contract.md
#	src/morphobricks_preprocessing/cli.py
#	src/morphobricks_preprocessing/manifest.py
#	src/morphobricks_preprocessing/masks.py
#	tests/test_masks.py

Copy link
Copy Markdown
Contributor Author

I added 9128250, so run_experiment refuses a non-bool missing_as_singleton/convert_tiffs before reading anything. Before this, the stricter writer only failed after the full run; a regression test covers it. Then I merged main (#40, #42, #43) in:

1120 passed, 1 skipped (test_discovery.py:472, root), ruff clean, CI 7/7 green on 3e833c8. Merging.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data-flow audit: minor findings (OME-Zarr scale, manifest flags, axis roles, CSV columns)

2 participants