Skip to content

PP-8b: refuse X-before-Y masks in the run; RunInputs names unrecorded files - #115

Merged
guijacquemet merged 2 commits into
mainfrom
claude/pp-8b-brick-checks
Nov 2, 2026
Merged

guijacquemet merged 2 commits into
mainfrom
claude/pp-8b-brick-checks

Conversation

@guijacquemet

@guijacquemet guijacquemet commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

This is a small fix PR that comes before PP-9 (owner decision, 2026-11-02). It comes from CellPatcher CP-2a (CellMigrationLab/CellPatcher#69, owner questions 1 and 2). It is based on main at 501c2dc. The version stays 0.3.4.

Summary

  • mask_checks.py: mask_issues now keeps the mask_layout error for a mask whose own header names X before Y.
    • Before, it dropped every mask_layout/mask_size_mismatch issue from find_masks, because the size comparison by role supersedes them. But a refused header also gives mask_layout, so that error disappeared too.
    • The run was then written, recording the mask as axes: "", shape: [].
    • New _header_refusals: it keeps a size-code issue only for the masks the header check gave no roles, so it never repeats a size comparison.
  • masks/stated.py: the mask_layout detail names the header axes: Cells/a.tif: header axes XY: X is stored before Y. When axes were stated it adds , stated …. The fix (numpy.swapaxes(mask, -1, -2)) is in the action. This reuses the existing code and message; no new code was added.
  • mask_record.py: mask_manifest raises ValueError for a mask without roles. A manifest can never record a mask with empty axes.
  • unrecorded.py (new, layer io): unrecorded_files(manifest, experiment_root, masks_root, output_folder=None) -> UnrecordedFiles(images, masks).
    • Images: what discover_experiment would analyse, less the recorded and the excluded images.
    • Masks:
      • masks-folder files that find_masks's own pairing (masks.files._folder_masks, whose images argument is now a Collection[str]) pairs with a recorded image;
      • label images of the recorded OME-Zarr images;
      • in both cases, less the recorded masks.
    • Not counted: files that discovery ignores or refuses, for example:
      • non-image and system files;
      • TIFFs outside a leaf folder or too deep;
      • masks that pair with no image;
      • masks of excluded images.
  • run_inputs.py:
    • RunInputs.open now runs _check_files: the existing fingerprint checks, then a refusal of a mask recorded without axes (its own message: "recorded … without axes … numpy.swapaxes … run preprocessing again", not "changed or missing"), then the unrecorded-files check.
    • New UnrecordedFilesError(RunInputsError). It has .files, and it pickles. Its message names up to five files per kind and says to run preprocessing again (morphobricks-preprocessing run --output <output folder>) or to exclude them (--exclude).
    • Exported from the package: UnrecordedFiles, UnrecordedFilesError, unrecorded_files.
  • Docs:
    • contract §9.1 (RunInputs) and §15 (mask header checks), plus the errors table row;
    • plan §10 milestone note, and plan §12 questions 50 and 51;
    • user guide: the mask_layout row, a new UnrecordedFilesError row, and §7 "Running again";
    • the AGENTS.md module map.

Decisions

  • Mask with X before Y in its own header: refused by check and run with the existing mask_layout code. Images are different: a header-named X-before-Y image is still read in canonical order (design change log, 2026-11-01), but masks are never reordered (design §3).
  • An older (rc1) record with empty axes: RunInputs.open refuses it for the whole run, not only at read_mask. Such a run would not be written today, so it is not a clean run.
  • Unrecorded-files check: RunInputs.open always runs it, and there is no opt-out. The reasons:
    • It reads no pixel and no TIFF header. It is one discover_experiment folder walk, plus, when a masks folder is recorded or given, an rglob of that folder, plus labels metadata for each recorded OME-Zarr image.
    • A check that bricks must remember to call could be forgotten.
    • The function is public too, for bricks or tools that want the list without the exception.

Notes for review

  • Behaviour change for bricks: opening a run now fails when an image or a paired mask was added after the run. A brick's tests that add images after make_clean_run and then open the run will now get UnrecordedFilesError. That is intended.
  • Label images (owner decision 3, changed in review c37d58f): never skipped. Without the zarr extra, RunInputs.open refuses a run holding OME-Zarr images, naming morphobricks-preprocessing[zarr]. A recorded OME-Zarr image whose labels group can no longer be read is its own RunInputsError naming the image (no longer listed as an unrecorded <image>/labels mask). unrecorded_files raises ZarrUnavailableError/NgffError for these.
  • Masks root given but not recorded: if a brick passes a masks_root while the run recorded no masks folder, every paired mask there counts as unrecorded. The run did not use them, so a rerun is needed.
  • Not covered: a mask stated X-before-Y on a plain stack also gives mask_layout through the same path, but it has no separate test, because a plain ?YX stack stated TXY contradicts the named Y first.
  • Cost: not benchmarked. The check only lists folders, so its cost is one directory walk per open.

Tests

  • New: tests/test_brick_checks.py has 10 tests.
  • On old code: with this file and main's source (501c2dc; UnrecordedFilesError aliased to RunInputsError so it imports), 9 fail:
    • both X-before-Y run tests (XY and TXY): the run was written;
    • mask_manifest did not raise;
    • the empty-axes record was not refused;
    • all five unrecorded-file tests: open did not raise. They cover an added image, an added image in a moved experiment, an added image with an excluded one beside it, a new mask in the masks folder, and a new OME-Zarr label image.
    • The 1 that passes is the guard: ignored or refused files (txt, png, .DS_Store, Thumbs.db, ._a.tif, a TIFF outside a leaf folder, an unpaired mask, a non-mask file in the masks folder) do not trigger the check.
  • Gate: Python 3.11, uv venv, .[dev,notebook,zarr], run as root.
    • ruff check .: all checks passed.
    • ruff format --check .: 122 files already formatted.
    • tools/check.py --rules: passes. The baseline and # noqa counts are unchanged, and tools/code_rules_baseline.json was not touched.
    • pytest: 1519 passed, 1 skipped. Main has 1509 passed, 1 skipped. The one skip is test_discovery.py:665 ("running with permissions that ignore file modes", as root, as expected).

§9 figures (measured on every new or changed function)

  • Measurement: the repo's tools/code_rules.py for executable lines and parameters; ruff --select C901 with max-complexity 0 for the complexity of each function.
  • ruff --select C901,PLR0913,PLR0915,ANN,D with the §9 limits passes on every changed module.
Function Exec lines Params C901
unrecorded.unrecorded_files 8 4 1
unrecorded._label_paths 14 2 5
unrecorded._folder_mask_paths 4 3 2
RunInputs.open 18 3 2
RunInputs._check_files 15 0 5
run_inputs._unrecorded_text / _without_axes_text / _shown 11 / 6 / 1 1 1
mask_checks._header_refusals 7 1 3
mask_record.mask_manifest 17 2 3
masks.stated.mask_roles 21 3 6
masks.files._folder_masks (annotation only) 40 3 11
  • Maximums over the changed code: 40 executable lines (limit 60), 4 parameters (limit 8), C901 11 (limit 15), and at most 31 statements (limit 50).
  • Annotations and docstrings: full ANN, NumPy docstrings for the public names (unrecorded_files, UnrecordedFiles, UnrecordedFilesError), and from __future__ import annotations.
  • Layers and reuse:
    • New module layer io is declared in pyproject.toml.
    • No helper is duplicated: discovery, pairing and pluralization reuse discover_experiment, _folder_masks, ngff_label_names and validation.plural.
  • File sizes:
File Before → after (lines)
run_inputs.py 438 → 508
mask_checks.py 111 → 131
mask_record.py 57 → 66
__init__.py 386 → 390
masks/stated.py 156 → 157
masks/files.py 342 → 343
unrecorded.py (new) 116
tests/test_brick_checks.py (new) 159

Review (independent reviewer, 2026-11-02)

  • Owner decisions 1–4 recorded in plan §12 question 51; decision 3 implemented with tests (c37d58f): missing zarr extra simulated with sys.modules["zarr"] = None, and an unreadable labels group. The 3 new tests fail on 2e1a956.
  • Gate (Python 3.12, uv venv, .[dev,notebook,zarr], root): ruff check and format clean (122 files); tools/check.py --rules passes, baseline untouched, # noqa 73 = main; pytest 1522 passed, 1 skipped (test_discovery.py:665, root). CI green.
  • Cost of the check: 300 images + 300 masks, RunInputs.open median 28 ms on main vs 88–103 ms here (unrecorded_files alone ~64 ms); 0 TIFF opens/TiffFile/imread calls during the check (instrumented).

Generated by Claude Code

… files

- A mask whose own header names X before Y (e.g. XY, TXY) is refused by
  check and run with mask_layout, naming the mask and its header axes
  and the numpy.swapaxes fix. mask_issues dropped this error together
  with the header size comparison it supersedes, so rc1 recorded such a
  mask with axes "". mask_manifest now refuses a mask without roles, and
  RunInputs.open refuses such an older record with its own message.
- unrecorded_files lists the images discover_experiment would analyse
  and the masks find_masks would pair with a recorded image (masks
  folder, OME-Zarr label images) that the run neither recorded nor
  excluded. RunInputs.open always runs it and raises
  UnrecordedFilesError (a RunInputsError naming the files). No pixel or
  TIFF header is read.
- Contract 9.1/15, plan questions 50 and 51, user guide, AGENTS map.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet marked this pull request as ready for review November 2, 2026 07:34
…asks for --masks

Owner decisions on the review questions (2 November 2026):
- Without the zarr extra, RunInputs.open refuses a run holding OME-Zarr
  images, naming morphobricks-preprocessing[zarr], instead of skipping
  their label images in the unrecorded-files check.
- A recorded OME-Zarr image whose labels group cannot be read is its own
  refusal naming the image, no longer listed as an unrecorded
  "<image>/labels" mask.
- Unrecorded masks (also a masks_root given to a run that recorded no
  masks folder): the message says to run preprocessing again with the
  masks folder (--masks); --exclude is offered only for images.
- Contract 9.1, user guide, plan question 51 record the decisions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet merged commit d35db1e into main Nov 2, 2026
7 checks passed
@guijacquemet
guijacquemet deleted the claude/pp-8b-brick-checks branch November 2, 2026 07:52
@guijacquemet
guijacquemet restored the claude/pp-8b-brick-checks branch November 2, 2026 07:53

Copy link
Copy Markdown
Contributor Author

Independent review: merged as d35db1e (merge commit)

Finding fixed in review, c37d58f (owner decision 3, plus the message for decision 2):

  • Before this fix, without the zarr extra the unrecorded-files check skipped OME-Zarr label images without a word. Now RunInputs.open refuses a run that holds OME-Zarr images and names morphobricks-preprocessing[zarr].
  • Before this fix, a recorded OME-Zarr image whose labels group could not be read came out as a misleading "unrecorded mask <image>/labels". It is now its own RunInputsError, which names the image. unrecorded_files raises ZarrUnavailableError or NgffError in these two cases.
  • When masks are unrecorded (including a masks_root passed for a run that recorded no masks folder), the message now says to run preprocessing again with the masks folder (--masks). --exclude is only offered when images are unrecorded.
  • 3 new regression tests. The missing extra is simulated with sys.modules["zarr"] = None. All 3 fail on 2e1a956.
  • Owner decisions 1–4 are recorded in plan §12 question 51. Contract §9.1 and the user guide are updated to match.

Verified:

  • Regression tests fail on old main: with main's source, 9 of the original 10 fail. The 10th is the guard test.
  • Uses the run's own rules: the check reuses discover_experiment, find_masks's _folder_masks and ngff_label_names. Exclusions are compared normalised, the same way the run records them. Excluded images and their masks, masks paired with no image, and files that discovery ignores are not counted.
  • No false positives: a clean RunInputs.open passes for:
    • make_clean_run with defaults, two mask types, ZCYX, TYX and a design;
    • a moved experiment plus moved masks (experiment_root/masks_root);
    • a single-folder layout with suffix masks and a confirmed plain stack;
    • OME-Zarr with labels, plus folder masks, plus an excluded image that has a mask.
    • The whole suite also opens runs in many more layouts.
  • X-before-Y masks: a run mixing a header-XY mask, a mask with Y/X sizes swapped and a size mismatch reports all three, with no drops or duplicates. mask_manifest refuses masks without roles.
  • UnrecordedFilesError: it pickles with .files intact.
  • No reads: instrumented TiffFile, imread and open of .tif files: 0 calls during the check on 300 images and 300 masks.
  • Cost: 300 images and 300 masks. RunInputs.open median is 28 ms on main against 88–103 ms here. unrecorded_files alone is about 64 ms (about 0.1 ms per file).

Gate (own worktree; uv venv, Python 3.12, .[dev,notebook,zarr], run as root):

  • ruff check and ruff format --check: clean (122 files).
  • tools/check.py --rules: passes. The baseline is unchanged, and the # noqa count is 73, the same as main.
  • pytest: 1522 passed, 1 skipped (test_discovery.py:665, expected as root).
  • CI: green on all 7 jobs.

§9 figures (the changed code):

  • Largest file touched: run_inputs.py, 541 lines (limit 600). unrecorded.py is 124 lines.
  • Largest function: masks.files._folder_masks, 40 executable lines and 31 statements. Only its annotation changed.
  • Highest complexity: C901 11 in _folder_masks. Among new functions the highest is 7 (RunInputs._check_files).
  • Most parameters: 4 (unrecorded_files).
  • ANN, from __future__ and docstrings are complete. The new module has its io layer declared.
  • ruff C901,PLR0913,PLR0915,ANN with the §9 limits passes on every changed module.

Process:

  • The version stays 0.3.4 and nothing was tagged. The owner publishes v0.4.0rc2.
  • The auto-deleted branch claude/pp-8b-brick-checks was restored at c37d58f.

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.

2 participants