Skip to content

Fix cross-brick test findings F1, F8, F9, F10, F11 - #139

Merged
guijacquemet merged 9 commits into
mainfrom
claude/fix-xbrick-preprocessing
Nov 3, 2026
Merged

guijacquemet merged 9 commits into
mainfrom
claude/fix-xbrick-preprocessing

Conversation

@guijacquemet

@guijacquemet guijacquemet commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes for findings F1, F8, F9, F10 and F11 of the cross-brick test of 3 November 2026 (MorphoFeature docs/restart/cross-brick-test-2026-11-03.md). The owner decided on 2026-11-03 to fix them before the round-2 audit. The version stays 0.4.0rc4. The decisions are recorded in plan §12 question 73.

Summary

  • mask_scan (F1): scan_masks now skips stated shared 2-D masks when it compares the planes of an image's masks, the same way masks.checks does. read_mask already returns the shared plane for every plane, so each relationship is still counted per plane and never across planes. Before this, masks refused a set that run accepts (per-plane Cells plus --mask-axes Nuclei=YX) with mask_planes_differ.
  • cli_run (F9): run no longer needs --axes when every readable image names all its axes (header_axes) and the suggested mapping gives each axis a role. It prints "Axis roles (as every image names them)", and the axis check still runs. Plain stacks still need --axes, and convert still requires it. When no mapping can describe the images, run without --axes prints the preflight's issues instead of the example --axes None (review).
  • run.pipeline (F10): if a rerun's manifest would differ only in written_at, the earlier time is kept. run_manifest.json then stays byte-identical, so pinned SHA-256s (CellPatcher, MorphoFeature) stay valid. Any real change still writes the new time, and the report records every rerun.
  • run.lock, conversion_issues, report, notebook_run (F11):
    • When the lock's holder is a process on this computer that is no longer running (POSIX, run.lock.looks_stale), the output_locked message says the lock looks stale and how to remove it (--remove-stale-lock, the notebook button, remove_stale_lock). The lock is never removed automatically. convert's copy-tree lock uses the same wording (review, coordinator decision 3).
    • A resumed run now says "No earlier run's rows were kept because …; the rows staged by the interrupted run were resumed (N images) and the others computed." It no longer says "All rows were computed".
  • cli_convert, size_checks, metadata, axis_review (F8):
    • (b) New option convert --output FOLDER (the output folder of the runs on the originals). The printed next commands name it, so they work. The recorded masks folder is also looked up there. Without --output, the default folder is named along with what to do if the runs used another one.
    • (a) inconsistent_dimensions and inconsistent_mapping now say that named-axis copies keep each image's stored order, so convert does not fix the problem.
    • (c) Documented in the user guide: save a pages-only plain stack again with its dimensions (for example as a Fiji hyperstack).
  • Docs: contract §9, §15 and §17, user guide, migration guide, CHANGELOG, AGENTS.md (an amendment to the CLI decision row, consistent with design v0.4 §3 "CLI --axes is needed only for plain stacks that are new or changed") and plan question 73.

Decisions

  • F8a: copies keep the stored order (§17), and the message explains this (coordinator: accepted). Reordering is left as an owner question.
  • F8c: a stated reshape for pages-only stacks is a follow-up (coordinator: accepted).
  • F9: AGENTS.md now notes that run uses the headers' roles when every image names them, and prints them.

Notes for review

  • A lock is reported as stale only for a process on the same host that is checked dead on POSIX. Other hosts and Windows keep the old message.
  • The F10 comparison uses RunManifest.to_dict(). A changed package version, or a manifest of an older schema, counts as a change.
  • In the notebook, the run card's "Every image was read because …" line now uses the same wording as the CLI.
  • §9 figures: files stay within the baseline (metadata.py 747, unchanged; largest other changed file mask_scan.py 578). Baseline: 38 entries, unchanged; review_axes 67→66 and _check_dimensions 30→29; noqa counts unchanged (ANN 51, PLR0913 3, C901 0, PLR0915 0). tools/check.py --rules is clean.

Tests

  • Regression tests that fail on main (c6d8aff): test_the_masks_command_scans_a_shared_2d_mask_with_per_plane_masks (F1), test_images_that_name_their_axes_need_no_axes_option (F9), test_a_rerun_that_changes_nothing_keeps_the_manifest_bytes (F10), test_the_summary_names_the_resumed_images (extended) and test_a_lock_left_by_a_killed_run_is_called_stale_and_kept (F11), test_the_printed_next_command_uses_the_output_folder_given_and_works and test_images_arranged_differently_are_told_that_convert_keeps_the_order (F8).
  • Review additions: test_the_convert_command_removes_a_stale_lock_only_on_request (extended: stale wording, lock kept), test_a_conversion_whose_folder_a_running_process_locks_is_not_called_stale, test_a_plain_stack_among_named_images_still_needs_axes.
  • Review gate, Python 3.12, .[dev,notebook,zarr]: ruff check and format clean, tools/check.py --rules clean, pytest 2064 passed, 4 skipped (expected on a Linux root container: case-sensitive file system ×3, permissions that ignore file modes ×1).

🤖 Generated with Claude Code

https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

The masks command refused per-plane Cells with a stated shared 2-D Nuclei
mask (mask_planes_differ) although the run accepts the set. Leave shared
masks out of the plane comparison, as masks.checks does; read_mask already
gives the shared plane for every plane.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
Design v0.4 §3: --axes is only needed for plain stacks. When every
readable image names all its axes and the suggested mapping gives each a
role, the first run uses the headers' roles; a plain stack still needs
--axes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
A no-op rerun rewrote run_manifest.json with only written_at changed, so
its SHA-256 (pinned by CellPatcher and MorphoFeature) changed. Keep the
earlier written_at when the manifest is otherwise identical; any real
change still writes the new time. The report still records the rerun.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
The lock refusal now says a lock left by a process that is no longer
running on this computer looks stale, and how to remove it (never
automatically). A resumed run no longer says all rows were computed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…keeps the order (F8)

convert takes --output, the output folder of the runs on the originals;
the printed next commands name it, so they work. Without it the default
folder is named with what to do if the runs used another one. Images
arranged differently are told that named-axis copies keep each image's
stored order (plan question 73).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
Contract §9, §15, §17, the user guide, the migration guide, the
changelog and AGENTS.md; plan §12 question 73 records the decisions and
the open F8 follow-ups (reordering copies, a stated page reshape).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
Move the arrangement message to size_checks and the F10 test to
test_resume; lower the baseline where the change shrank functions.

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 3, 2026 09:50
…review of #139)

convert's output_locked still said another conversion was writing when the holder was a
process on this computer that is no longer running. It now uses the run's wording through
the shared lock.looks_stale; the lock is never removed without --remove-stale-lock.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
… --axes None (F9, review of #139)

The run's --axes help still said it was needed for the first run. Without --axes, images
that no mapping describes (a plain stack among named images) printed the example
'--axes None'; the preflight's issues are printed instead, as with --axes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet merged commit e24f334 into main Nov 3, 2026
7 checks passed
@guijacquemet
guijacquemet deleted the claude/fix-xbrick-preprocessing branch November 3, 2026 11:07
@guijacquemet
guijacquemet restored the claude/fix-xbrick-preprocessing branch November 3, 2026 11:07

Copy link
Copy Markdown
Contributor Author

Independent review, merged as e24f334 (merge commit; the branch was restored on the PR head 9c1d018).

Verified adversarially

  • F1: I built a case with a known answer: a 3-plane image, per-plane Cells (plane 0 covers nucleus 1, plane 1 covers no nucleus, plane 2 has two cells and one covers nucleus 1), and a shared 2-D Nuclei mask with 2 nuclei (Nuclei=YX). The result was children 6, inside 2, outside 4, parents 4, parents_with_1_child 2 and child_pixels 24. Every count matches the expected values, so the comparison is per plane and never across planes. With every mask shared, the set is scanned once (1 child, inside 1). The existing per-plane tests pass.
  • F9: I ran the same named experiment (ZCYX + CYX) with --axes ZCYX and without it. The manifests are identical apart from written_at. The report's images roles and axes_from (header) are identical; only output_folder differs. Each of these still needs --axes, or is refused, and nothing is guessed: plain stacks, a plain stack among named images, a shaped QYX. TZYX works without --axes, and the axis check still prints. convert still has --axes as required. AGENTS.md and design v0.4 §3 ("CLI --axes is needed only for plain stacks that are new or changed") agree.
  • F10: the manifest SHA and written_at stay unchanged on a no-op rerun, and also on a no-op --mode full. A new SHA and time come with each of: a design edit, a calibration statement, masks added, a mask changed, --bins, a touched image, --exclude. The report's manifest_sha256 always equals the file's SHA, and the report is rewritten on every rerun (mode, reused). The conversion record follows the same keep-if-same rule, so pins held by CellPatcher (manifest_sha256) and MorphoFeature (inputs.manifest_sha256) stay valid across no-op reruns.
  • F11: stale wording appears only for a holder on the same host whose process is gone (POSIX). A live PID, or another host, still gets "Another …". The lock is never removed without --remove-stale-lock.
  • F8: I ran the printed check and run commands end to end, both with --output and with the default folder. All exited with 0, and a rerun of the printed run also exited with 0.

Review findings, fixed

  • 15c4633: convert's copy-tree lock now uses the stale wording too, through the shared run.lock.looks_stale (coordinator decision 3). Tests: stale and kept; a live holder is not called stale.
  • 9c1d018: the run --axes help still said "Needed for the first run". Also, run without --axes printed --axes None when no mapping describes the images (for example a plain stack among named ones); it now prints the preflight's issues. Regression test added.

Gate (Python 3.12, .[dev,notebook,zarr]): ruff check and format clean; tools/check.py --rules clean; pytest 2064 passed, 4 skipped. All 4 skips are expected on a Linux root container: 3 because the file system is case-sensitive, 1 because root permissions ignore file modes. CI is green on all 7 jobs.

§9 figures: no file over its baseline (metadata.py 747, unchanged; the largest other changed file is mask_scan.py with 578). The baseline still has 38 entries; review_axes went from 67 to 66 and _check_dimensions from 30 to 29. noqa counts are unchanged: ANN 51, PLR0913 3, C901 0, PLR0915 0.

Left for the owner (already recorded in question 73): reordering copies into the common order (F8a), and a stated reshape for pages-only stacks (F8c). One minor point outside this PR: the printed next commands do not quote paths that contain spaces.


Generated by Claude Code

guijacquemet pushed a commit that referenced this pull request Nov 3, 2026
Q66 confirmed by the owner (3 November 2026); Q73's follow-up decided: neither a
stated reshape nor a reorder in v0.4, kept in issue #144; Q74 records the
tie-break and the consumers checked. The changelog gains a section for rc5 (PRs
#139, #140, #142, #143) with migration notes: mounts refused, position_id
required in v2, micrometre conversion within 1 ULP, --verify-content, and the
tie-break. The migration guide gets the same.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet deleted the claude/fix-xbrick-preprocessing branch November 14, 2026 08:20
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