From 36b9f865e57f6f35ced70cdd461e995e047fa030 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 07:53:09 +0000 Subject: [PATCH 1/9] PP-8c: output files get the mode a plain open() gives Atomic writes went through tempfile.mkstemp, so every output was 0600 whatever the umask and colleagues could not read results in a shared folder. A new file now gets 0666 & ~umask; a file that existed keeps its own mode. The table (written by the normalization brick's writer into the staging folder) is given its mode when it is published. POSIX only. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 9 ++ docs/plan.md | 11 +++ docs/user_guide.md | 6 ++ src/morphobricks_preprocessing/_io.py | 63 ++++++++++++++ src/morphobricks_preprocessing/run/publish.py | 3 +- tests/test_output_permissions.py | 87 +++++++++++++++++++ 6 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 tests/test_output_permissions.py diff --git a/docs/contract.md b/docs/contract.md index 6b41b493..99dd6bcd 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -144,6 +144,15 @@ previous state, including the absence of files on a first run. If restoration fa backup folder is retained and its location is reported for manual recovery. Failures to remove temporary files after a successful publication are logged. +**File permissions (v0.4.0, plan §12 question 50).** Every file the brick writes (the +table, the manifest, the design, the report files, `calibration.csv` and +`mask_relationships.json`) gets the permissions a plain `open()` would give it: a new file +`0666` without the process's umask (`0644` with the usual umask `022`), so colleagues can +read the results in a shared folder; a file that already exists keeps its own mode, as the +user set it. Before v0.4.0 every output was `0600` (owner only) whatever the umask, because +the temporary file of the atomic write kept its private mode; such files keep `0600` until +the user changes them (`chmod a+r`) or deletes them. On Windows file modes do not apply. + The two replacements are not jointly atomic: forced process termination between them can leave a mismatched pair, detected by `csv_sha256` on the next run and rebuilt. Only one run writes to a given output folder at a time, enforced by a lock (§9.2). This write protocol diff --git a/docs/plan.md b/docs/plan.md index 8fcb443b..b4e6b62b 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1122,3 +1122,14 @@ Raised during review; to be decided before the milestones that need them: (owner, 1 November 2026). The manifest records `calibration_sha256`, checked by `RunInputs`; a changed file is an input change that recomputes no row. Y and X are stated per role and never exchanged. Contract §9.3.* +50. Which permissions do the output files get (PP-8c, from a run on real data)? + *Decided (2 November 2026, owner): the mode a plain `open()` gives. The atomic write's + temporary file (`tempfile.mkstemp`) is private, so every output was `0600` whatever the + umask, and colleagues could not read the results in a shared lab folder. Now a new file + gets `0666 & ~umask` (the umask read from `/proc/self/status` where the system gives + it, else by setting it under a lock, to the most restrictive value while it is set), and + a file that already exists keeps its own mode, as the user set it: the run never widens + or narrows what the user chose. Files written `0600` by earlier versions therefore stay + so until the user changes them; the user guide says how. The table, written by the + normalization brick's writer into the staging folder, is given its mode when it is + published. Windows: unchanged (no such modes). Contract §9.* diff --git a/docs/user_guide.md b/docs/user_guide.md index fb7f2051..4b5975b5 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -568,6 +568,12 @@ the new outputs and copies of the earlier pair. If restoration also fails (for e another program locks an output file), the error names a backup folder: close programs using the output files and restore those backups before running again. +Every output file gets the permissions a file you save yourself would get (your umask; +usually readable by everyone, writable by you), so colleagues can open the results in a +shared folder. A file that already exists keeps the permissions you gave it. Outputs +written by versions before v0.4.0 are readable by their owner only: make them readable +with `chmod a+r /*`, or delete them before running again. + Only one run at a time can use an output folder: a run holds a lock file (`.morphobricks-preprocessing.lock`) and a second run is refused (`output_locked`). If the process is forcibly stopped, its lock stays; remove it with `--remove-stale-lock` once you diff --git a/src/morphobricks_preprocessing/_io.py b/src/morphobricks_preprocessing/_io.py index 044d6098..79d1f91f 100644 --- a/src/morphobricks_preprocessing/_io.py +++ b/src/morphobricks_preprocessing/_io.py @@ -4,15 +4,26 @@ then moved over the target with :func:`os.replace`, so a reader never sees half a file and an interrupted write leaves the earlier file as it was (contract §9). One function does it, so the rule is not copied per output file. + +A temporary file is created readable by its owner only, so before it replaces the target +it is given the mode the file is to have (:func:`give_output_mode`): the mode of the file +it replaces, kept as the user set it, or for a new file the mode a plain ``open()`` gives, +``0666`` without the process's umask, so colleagues can read the results in a shared +folder. On Windows, where files have no such modes, nothing is changed. """ from __future__ import annotations import os import shutil +import stat import tempfile +import threading from pathlib import Path +_UMASK_LOCK = threading.Lock() +"""Serialises reading the umask by setting it, the only portable way to read it.""" + def write_text_atomic(path: str | os.PathLike, text: str, *, parents: bool = False) -> Path: """Write UTF-8 text to ``path`` atomically, with the line endings as given. @@ -39,6 +50,7 @@ def write_text_atomic(path: str | os.PathLike, text: str, *, parents: bool = Fal try: with os.fdopen(handle, "w", newline="", encoding="utf-8") as stream: stream.write(text) + give_output_mode(temporary, path) os.replace(temporary, path) except BaseException: Path(temporary).unlink(missing_ok=True) @@ -46,6 +58,57 @@ def write_text_atomic(path: str | os.PathLike, text: str, *, parents: bool = Fal return path +def new_file_mode() -> int: + """Return the mode a plain ``open()`` gives a new file: ``0666`` without the umask. + + Returns + ------- + int + The permission bits, e.g. ``0o644`` with the usual umask ``022``. + """ + return 0o666 & ~_umask() + + +def _umask() -> int: + """Return the process's umask, reading it without changing it where the system can. + + Linux gives it in ``/proc/self/status``. Elsewhere it is read by setting it, under a + lock; while it is set, it is the most restrictive one, so a file another thread + creates meanwhile is never more readable than asked. + """ + try: + with open("/proc/self/status", encoding="ascii") as status: + for line in status: + if line.startswith("Umask:"): + return int(line.split()[1], 8) + except (OSError, ValueError, IndexError): + pass + with _UMASK_LOCK: + umask = os.umask(0o077) + os.umask(umask) + return umask + + +def give_output_mode(temporary: str | os.PathLike, target: str | os.PathLike) -> None: + """Give a temporary file the mode it is to have once it replaces ``target``. + + Parameters + ---------- + temporary + The finished temporary file. + target + The file it replaces: its mode is kept when it exists, else the mode a plain + ``open()`` would give (:func:`new_file_mode`). Nothing is changed on Windows. + """ + if os.name != "posix": + return + try: + wanted = stat.S_IMODE(os.stat(target).st_mode) + except OSError: # no such file (or a link to none): a new file + wanted = new_file_mode() + os.chmod(temporary, wanted) + + def free_space(path: str | os.PathLike) -> int | None: """Return the free space on the drive holding ``path`` (or its nearest existing parent). diff --git a/src/morphobricks_preprocessing/run/publish.py b/src/morphobricks_preprocessing/run/publish.py index 2e1663e3..1a8faa87 100644 --- a/src/morphobricks_preprocessing/run/publish.py +++ b/src/morphobricks_preprocessing/run/publish.py @@ -20,7 +20,7 @@ OUTPUT_FILENAME, ) -from .._io import write_text_atomic +from .._io import give_output_mode, write_text_atomic from ..design import ( DESIGN_FILENAME, ) @@ -108,6 +108,7 @@ def _publish_outputs( published: list[str] = [] try: for name in filenames: + give_output_mode(staged / name, output / name) os.replace(staged / name, output / name) published.append(name) except BaseException: diff --git a/tests/test_output_permissions.py b/tests/test_output_permissions.py new file mode 100644 index 00000000..ffc2f8b3 --- /dev/null +++ b/tests/test_output_permissions.py @@ -0,0 +1,87 @@ +"""PP-8c item 1: output files get the permissions a normal ``open()`` would give them. + +Every output is written atomically (a temporary file, then a replace). A temporary file +is created readable by its owner only (0600), so before this fix every output was 0600 +whatever the umask, and colleagues could not read the results in a shared lab folder. +Now a new file gets ``0666 & ~umask``, and a file that existed keeps its own mode. +""" + +import os +import stat +import sys + +import pytest + +from morphobricks_preprocessing import _io, calibration_entry, run_experiment, write_calibration +from morphobricks_preprocessing.calibration import Calibration +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.testing import make_clean_run + +pytestmark = pytest.mark.skipif( + sys.platform == "win32", reason="POSIX file modes: Windows files have no umask modes" +) + + +@pytest.fixture +def umask_022(): + previous = os.umask(0o022) + try: + yield + finally: + os.umask(previous) + + +def mode(path): + return stat.S_IMODE(path.stat().st_mode) + + +def test_new_file_mode_follows_the_umask(umask_022): + new_file_mode = _io.new_file_mode # added by the fix + assert new_file_mode() == 0o644 + os.umask(0o002) + assert new_file_mode() == 0o664 + assert os.umask(0o002) == 0o002 # reading it leaves the umask as it was + + +def test_an_atomic_write_gives_the_mode_of_a_normal_open(tmp_path, umask_022): + written = _io.write_text_atomic(tmp_path / "a.txt", "x") + with open(tmp_path / "b.txt", "w") as stream: + stream.write("x") + assert mode(written) == mode(tmp_path / "b.txt") == 0o644 + + +def test_a_file_that_existed_keeps_its_mode(tmp_path, umask_022): + path = tmp_path / "a.txt" + path.write_text("before") + path.chmod(0o640) + _io.write_text_atomic(path, "after") + assert path.read_text() == "after" + assert mode(path) == 0o640 + + +def test_every_output_of_a_run_is_readable_by_others(tmp_path, umask_022): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8), mask_types=("Cells",), design=True) + write_calibration(Calibration((calibration_entry(".", "X", 0.5, "um"),)), clean.output) + args = ["masks", str(clean.experiment), "--output", str(clean.output)] + assert main([*args, "--masks", str(clean.masks)]) == 0 + files = sorted(path for path in clean.output.iterdir() if path.is_file()) + names = {path.name for path in files} + assert { + "plane_histograms.csv", + "run_manifest.json", + "discovery_report.json", + "discovery_report.txt", + "experiment_design.csv", + "calibration.csv", + "mask_relationships.json", + } <= names + assert {path.name: mode(path) for path in files} == {name: 0o644 for name in names} + + +def test_a_rerun_keeps_the_modes_the_user_set(tmp_path, umask_022): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8)) + table = clean.output / "plane_histograms.csv" + table.chmod(0o600) + assert run_experiment(output_folder=clean.output, mode="full").written + assert mode(table) == 0o600 + assert mode(clean.output / "run_manifest.json") == 0o644 From d1dd39b60811fc81dcc107e8e7b8f3337bf9156e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:00:05 +0000 Subject: [PATCH 2/9] PP-8c: no pixel size for an axis the image does not have A CYX max projection kept its stack's z-step (HPMEC: Z 0.43 um) and the run recorded it as the image's Z size. A Z or T size is now recorded only for an image whose roles include that axis (calibration.fit_to_roles, applied after the axis review in run and check). A calibration.csv statement for an absent axis is the warning calibration_axis_absent naming the images, never a recorded size. The report text counts Z and T sizes for the images with the axis. RowKey has no pixel sizes, so earlier rows are reused. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 6 +- docs/plan.md | 15 ++ docs/user_guide.md | 8 +- src/morphobricks_preprocessing/calibration.py | 75 ++++++++- src/morphobricks_preprocessing/cli.py | 10 +- .../discovery_report.py | 26 +++- .../run/pipeline.py | 9 +- .../run/preflight.py | 27 +++- tests/test_absent_axis_sizes.py | 144 ++++++++++++++++++ 9 files changed, 304 insertions(+), 16 deletions(-) create mode 100644 tests/test_absent_axis_sizes.py diff --git a/docs/contract.md b/docs/contract.md index 2052794c..543b5d90 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -257,12 +257,12 @@ the bricks' tests. | Topic | Rule | |---|---| -| Source per axis | For X, Y and Z, an image's size comes from its header (`header`) or from a statement in the output folder's `calibration.csv` (`stated`, or `pixels`: deliberately uncalibrated). The time step comes from the header only. An axis with neither is uncalibrated: no value, no source, never a default. | +| Source per axis | For X, Y and Z, an image's size comes from its header (`header`) or from a statement in the output folder's `calibration.csv` (`stated`, or `pixels`: deliberately uncalibrated). The time step comes from the header only. An axis with neither is uncalibrated: no value, no source, never a default. A Z or T size is recorded only for an image whose roles include Z or T (v0.4.0, plan §12 question 53): a max projection's header may still give the z-step of its stack, and that is not a size of the projection (a singleton role of an axis the file lacks, `missing_as_singleton`, does not count). | | File | UTF-8 CSV, columns `target,axis,size,unit`, one statement per line. `target`: `.` for the whole experiment, or a folder (a condition or a repeat) or an image, relative to the experiment folder with `/`, compared exactly (backslashes, a leading `./` and a trailing `/` are normalised; an absolute path, a drive, or an empty, `.` or `..` component elsewhere is refused). `axis`: `X`, `Y` or `Z`. `size`: a positive finite number in plain ASCII decimal notation (`0.65`, `1e-3`; not `0,65` or `1_0`); empty with the unit `pixels`. `unit`: a length unit of decision D-a2 in any spelling `space_unit` reads (written back as its OME-NGFF name), or `pixels`. The reader (`read_calibration`) is strict: other columns, a value missing or extra, outer whitespace, a target outside the experiment folder, an unknown axis or unit, a size that is not a positive number, or two statements for one target and axis are refused naming the line (`CalibrationFormatError`). A UTF-8 byte-order mark and Windows line endings are read. | | Precedence | The most specific statement covering an image applies: the image's own, then the deepest folder's, then the whole experiment's. Statements are per role: Y and X are stated and recorded on their own, and nothing exchanges them. | | Writing | A run reads `calibration.csv` and never writes, replaces or removes it, so it survives `full`, stopped and failed runs. The user writes it by hand, with `write_calibration(calibration, output_folder)` (holding the output folder's lock; `OutputLockedError` while a run holds it) or with the CLI `calibrate` (`--size TARGET AXES SIZE UNIT`, `--pixels TARGET AXES`, `--remove TARGET AXES`; without them it prints the statements), which reads, changes and writes the file under the lock (`run.calibrate.update_calibration`), so two changes never overwrite each other. | -| Checks | `calibration_unreadable` (error: the file cannot be read; left as it is); `calibration_target_not_found` (error: a target that is neither a folder nor an image of the experiment, excluded images included); `calibration_differs_from_header` (warning, naming each image and axis with both sizes: a stated size not the header's by the D-a rule, or `pixels` where the header gives a size; z-steps only for images with more than one depth slice; the stated size is used). The §11 pixel-size checks run on the resolved sizes; a header size in an unsupported unit is not reported on an axis that is stated. `check` and the notebook's folder check (`preflight`) use the same sizes. | -| Recording | Each manifest entry's `pixel_sizes` records the resolved sizes with their source (§9); the manifest records `calibration_sha256`; the report shows each image's sources and the statements. Changing the calibration recomputes no row: the next `restart` records the new sizes, reuses every row and names the change (§9.2). `RunInputs` refuses a `calibration.csv` changed, removed or added since the run. | +| Checks | `calibration_unreadable` (error: the file cannot be read; left as it is); `calibration_target_not_found` (error: a target that is neither a folder nor an image of the experiment, excluded images included); `calibration_differs_from_header` (warning, naming each image and axis with both sizes: a stated size not the header's by the D-a rule, or `pixels` where the header gives a size; z-steps only for images with more than one depth slice; the stated size is used); `calibration_axis_absent` (warning: a statement covers images without that axis, for example a Z size for the whole experiment with 2D images; the images are named, and the size is not recorded for them). The §11 pixel-size checks run on the resolved sizes; a header size in an unsupported unit is not reported on an axis that is stated. `check` and the notebook's folder check (`preflight`) use the same sizes. | +| Recording | Each manifest entry's `pixel_sizes` records the resolved sizes with their source (§9); the manifest records `calibration_sha256`; the report shows each image's sources and the statements, and its text counts the Z and T sizes of the images with that axis. Changing the calibration recomputes no row: the next `restart` records the new sizes, reuses every row and names the change (§9.2). `RunInputs` refuses a `calibration.csv` changed, removed or added since the run. | ## 10. Converting TIFF images to OME-Zarr (removed in v0.4.0) diff --git a/docs/plan.md b/docs/plan.md index 72c43ea5..467b9f2e 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1169,3 +1169,18 @@ Raised during review; to be decided before the milestones that need them: so until the user changes them; the user guide says how. The table, written by the normalization brick's writer into the staging folder, is given its mode when it is published. Windows: unchanged (no such modes). Contract §9.* +53. Is a pixel size recorded for an axis the image does not have (PP-8c, from a run on + real data)? + *Decided (2 November 2026, owner): no. MorphoRelate's HPMEC max projections (`CYX`) + keep the z-step of their stacks in their ImageJ metadata (0.43 µm), and the run + recorded it as their Z size from the header. A Z or T size is now recorded only for an + image whose roles include that axis (`calibration.fit_to_roles`, after the axis + review; a singleton role of a missing axis does not count); otherwise the axis has no + value and no source. A `calibration.csv` statement for an axis an image lacks is + neither refused nor recorded: the warning `calibration_axis_absent` names the images + (refusing would make a whole-experiment Z statement impossible in an experiment that + mixes stacks and 2D images; the question 49 rule that a statement matching nothing is + an error is about targets, not axes). The report's text counts Z and T sizes for the + images with the axis. The `RowKey` does not include pixel sizes, so earlier rows are + reused (no row depends on a size, question 49): the next `restart` leaves the table + byte-identical and rewrites the manifest without the absent sizes. Contract §9.3.* diff --git a/docs/user_guide.md b/docs/user_guide.md index 8a370365..8d723a04 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -509,8 +509,11 @@ A run reads the file and never changes or removes it (also with `--mode full`, o stops). The next run records each image's sizes with their source (`header`, `stated` or `pixels`), names the change in its report, and recomputes no row. A stated size that differs from the metadata is a warning (`calibration_differs_from_header`); the stated size -is used. In Python, `write_calibration(calibration, output_folder)` writes the file -(see §9). +is used. A Z (or time) size is recorded only for images that have that axis: a max +projection whose metadata still give the z-step of its stack gets no Z size, and a +statement for an axis some images lack is a warning naming them +(`calibration_axis_absent`). In Python, `write_calibration(calibration, output_folder)` +writes the file (see §9). ## 6. Output files @@ -659,6 +662,7 @@ continues. | `different_pixel_sizes` | Warning | The physical pixel size or z-step (from the metadata or `calibration.csv`) differs between images by more than 0.1 % (for example different objectives). Images outside the largest group of matching sizes are listed; when sizes only change by small steps from image to image, none is listed. Check the microscope settings if this is not intended. | | `missing_pixel_size` | Warning | Some images have a pixel size (from the metadata or `calibration.csv`) and others do not, so it cannot be checked; images stated as measured in pixels count as without one, and the message says how many. Export them again with metadata, or state their sizes with `calibrate`, if needed. | | `unsupported_pixel_size_unit` | Warning | An image gives its pixel size or z-step in a unit that is not converted (for example inches); only pm, Å, nm, µm, mm, cm and m are. The message names each image, axis and unit. Those sizes are not used: the image is treated as uncalibrated on those axes. Export it again with the pixel size in a supported unit, or state it with `calibrate`, if the physical size matters. Not reported for an axis whose size is stated. | +| `calibration_axis_absent` | Warning | `calibration.csv` states a size for an axis some images do not have (for example a Z size for 2D images or projections); the message names them. No size is recorded for an axis an image does not have. Give the statement a narrower target, or no action if it is meant for the other images. | | `calibration_differs_from_header` | Warning | `calibration.csv` states a pixel size (or `pixels`) that differs from an image's metadata; the message names each image and axis with both sizes. The stated size is used. Correct or remove the statement if the metadata are right. | | `calibration_target_not_found` | Error | `calibration.csv` names a folder or image that is not in the experiment (names are compared exactly, case included). Correct the target, or remove the line. | | `calibration_unreadable` | Error | `calibration.csv` cannot be read; the message names the line and what is wrong. The file is left as it is: correct it, or remove it to use the sizes in the metadata. | diff --git a/src/morphobricks_preprocessing/calibration.py b/src/morphobricks_preprocessing/calibration.py index c4327884..94e9733f 100644 --- a/src/morphobricks_preprocessing/calibration.py +++ b/src/morphobricks_preprocessing/calibration.py @@ -19,6 +19,10 @@ ``calibration_differs_from_header`` naming the images, and the statement is used. - **Unknown targets.** A statement whose target is neither a folder nor an image of the experiment is the error ``calibration_target_not_found``: it is never dropped silently. +- **Only axes the image has.** A Z or T size is recorded only for an image with that role + (:func:`fit_to_roles`): a max projection keeps the z-step of its stack in its metadata, + which is not a size of the projection. A statement for an axis an image lacks is the + warning ``calibration_axis_absent`` naming the images, and is not recorded for them. - **No row depends on it.** Changing the calibration recomputes no row. """ @@ -26,8 +30,8 @@ import math import re -from collections.abc import Iterable, Mapping, Sequence -from dataclasses import dataclass, field +from collections.abc import Collection, Iterable, Mapping, Sequence +from dataclasses import dataclass, field, replace from types import MappingProxyType from typing import TYPE_CHECKING @@ -244,7 +248,8 @@ class CalibrationReview: The SHA-256 of the file's bytes as read, or None without one. issues ``calibration_unreadable``, ``calibration_target_not_found`` and - ``calibration_differs_from_header``. + ``calibration_differs_from_header``; after :func:`fit_to_roles`, also + ``calibration_axis_absent``. """ sizes: Mapping[str, tuple[RecordedSize, ...]] = field(default_factory=dict) @@ -316,6 +321,70 @@ def resolve_sizes(meta: TiffMetadata, calibration: Calibration) -> tuple[Recorde return tuple(resolved) +def fit_to_roles( + review: CalibrationReview, roles: Mapping[str, Collection[str | None]] +) -> tuple[CalibrationReview, tuple[ValidationIssue, ...]]: + """Keep a Z or T size only for the images that have that role. + + The header of a max projection still gives the z-step of its stack, and a statement for + the whole experiment covers every image: neither is a size of an axis the image does not + have. Such an axis is recorded with no value and no source; a statement for it is the + warning ``calibration_axis_absent`` naming the images (the statement still sizes the + images that have the axis). + + Parameters + ---------- + review + The calibration resolved from the headers and ``calibration.csv``. + roles + ``{relative path: the roles the image is read with}`` (its axes' roles, without the + singleton roles of axes it lacks); images left out keep their sizes. + + Returns + ------- + tuple + The review with those sizes removed and the warning among its issues, and the new + issues alone (the warning, or nothing). + """ + statements = review.calibration or Calibration() + sizes, absent = dict(review.sizes), {} + for path, found in review.sizes.items(): + if path not in roles: + continue + lacking = [role for role in ("Z", "T") if role not in roles[path]] + sizes[path] = tuple(RecordedSize(s.role) if s.role in lacking else s for s in found) + for axis in lacking: + if axis in CALIBRATION_AXES and statements.statement(path, axis) is not None: + absent.setdefault(path, []).append(axis) + issues = _absent_axis_issues(absent) + fitted = replace(review, sizes=sizes, issues=(*review.issues, *issues)) + return fitted, issues + + +def _absent_axis_issues(absent: Mapping[str, list[str]]) -> tuple[ValidationIssue, ...]: + """Warn about statements for an axis the images they cover do not have.""" + if not absent: + return () + paths = sorted(absent) + shown = [f"{path} (no {' or '.join(absent[path])})" for path in paths[:_SHOWN]] + more = len(paths) - len(shown) + axes = " and ".join(sorted({axis for found in absent.values() for axis in found})) + return ( + warning( + "calibration_axis_absent", + f"calibration.csv states a {axes} size for {plural(len(paths), 'image')} that " + "have no such axis: " + + "; ".join(shown) + + (f"; and {more} more" if more else "") + + ". No size is recorded for an axis an image does not have.", + paths, + "No action needed if the statement is meant for other images (for example a " + "statement for the whole experiment). Otherwise give it a narrower target, or " + "remove it.", + ), + ) + + def _unknown_targets(calibration: Calibration, images: list[str]) -> list[ValidationIssue]: """Refuse statements whose target is neither a folder nor an image of the experiment.""" unknown = sorted( diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index 6d4a368a..aba8231f 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -64,6 +64,7 @@ write_check_report, ) from .run.lock import lock_issue, output_lock +from .run.preflight import fit_calibration from .validation import has_errors __all__ = ["build_parser", "main", "parse_size"] @@ -220,6 +221,10 @@ def _remove_stale_lock(args: argparse.Namespace) -> bool: def _check(args: argparse.Namespace) -> int: """Print what the preflight found and the suggested roles, and write the report.""" pre = command_preflight(args) + review = None + if pre.suggested_mapping is not None: + review = review_axes(pre.metadata, pre.suggested_mapping) + pre, _ = fit_calibration(pre, review) # no Z or T size for images without one print(preflight_summary(pre)) if pre.discovery.leaves: print() @@ -238,9 +243,8 @@ def _check(args: argparse.Namespace) -> int: print(format_issues(pre.issues)) if pre.dataset.repeats: _print_design(args, pre) - axis_errors, review = (), None - if pre.suggested_mapping is not None: - review = review_axes(pre.metadata, pre.suggested_mapping) + axis_errors = () + if review is not None: # The suggested roles are the header's own, so they never contradict it; 'run' # checks the roles given with --axes. axis_errors = review.axis_errors diff --git a/src/morphobricks_preprocessing/discovery_report.py b/src/morphobricks_preprocessing/discovery_report.py index 53bfc2ca..2045fb9c 100644 --- a/src/morphobricks_preprocessing/discovery_report.py +++ b/src/morphobricks_preprocessing/discovery_report.py @@ -18,7 +18,7 @@ from __future__ import annotations from collections import Counter -from collections.abc import Mapping, Sequence +from collections.abc import Iterable, Mapping, Sequence from dataclasses import dataclass, field from pathlib import Path from typing import TYPE_CHECKING @@ -364,12 +364,36 @@ def _image_lines(data: Mapping) -> list[str]: f"Pixel sizes: {kinds[HEADER]} from the header, {kinds[STATED]} stated, " f"{kinds[PIXELS]} measured in pixels as stated, {kinds[None]} uncalibrated" ) + lines.extend(_axis_size_line(images, axis) for axis in ("Z", "T") if _with_axis(images, axis)) statements = data["calibration"]["statements"] if data["calibration"]["file"] is not None: lines.append(f" calibration.csv: {plural(len(statements), 'statement')}") return lines +def _with_axis(images: Iterable[Mapping], axis: str) -> list[Mapping]: + """Return the images read with ``axis`` among their roles.""" + return [image for image in images if axis in (image["roles"] or "")] + + +def _axis_size_line(images: Iterable[Mapping], axis: str) -> str: + """Count how the Z or T size of the images with that axis is known; the others have none.""" + images = list(images) + having = _with_axis(images, axis) + sources = Counter( + image["pixel_sizes"].get(axis, {}).get("source") + if isinstance(image["pixel_sizes"], Mapping) + else None + for image in having + ) + name = "Z step" if axis == "Z" else "Time step" + return ( + f" {name}: {sources[HEADER]} from the header, {sources[STATED]} stated, " + f"{sources[PIXELS]} in pixels, {sources[None]} unknown; " + f"{len(images) - len(having)} without {axis} (no size recorded)" + ) + + def _calibration_kind(sizes: Mapping | str) -> str | None: """Return how an image's pixel size (X by Y) is known: header, stated, pixels or None.""" if sizes == "uncalibrated": diff --git a/src/morphobricks_preprocessing/run/pipeline.py b/src/morphobricks_preprocessing/run/pipeline.py index 5784d4fb..d4191354 100644 --- a/src/morphobricks_preprocessing/run/pipeline.py +++ b/src/morphobricks_preprocessing/run/pipeline.py @@ -56,7 +56,7 @@ from ..validation import ValidationIssue, error, has_errors, warning from .lock import OutputLockedError, lock_issue, output_lock from .modes import FULL, RESTART, check_mode -from .preflight import Preflight, preflight +from .preflight import Preflight, fit_calibration, preflight from .publish import _staged_outputs, write_report from .recorded import ( GivenInputs, @@ -226,10 +226,13 @@ def _locked_run(given: GivenInputs, output: Path, options: _Options) -> RunResul mask_axes=inputs.mask_axes or None, ) design, design_problems, design_changed = _design(options.design, output, previous[0]) - calibrated = calibration_change(usable(previous[0]), pre.calibration) - inputs = replace(inputs, changes=(*inputs.changes, *design_changed, *calibrated)) checked = header_checks(pre, inputs, design, options.memory_limit) checked.issues.extend(design_problems) + if checked.review is not None: + pre, absent = fit_calibration(pre, checked.review) + checked.issues.extend(absent) + calibrated = calibration_change(usable(previous[0]), pre.calibration) + inputs = replace(inputs, changes=(*inputs.changes, *design_changed, *calibrated)) run = _Run(output, pre, inputs, options.mode, checked, previous, found) recorded = usable(previous[0]) base = RunOutcome( diff --git a/src/morphobricks_preprocessing/run/preflight.py b/src/morphobricks_preprocessing/run/preflight.py index c76e50c3..bc8c106a 100644 --- a/src/morphobricks_preprocessing/run/preflight.py +++ b/src/morphobricks_preprocessing/run/preflight.py @@ -15,10 +15,11 @@ from ..axes import ( AxisMapping, + AxisReview, file_mapping_for, prefill_experiment_mapping, ) -from ..calibration import CalibrationReview, review_calibration +from ..calibration import CalibrationReview, fit_to_roles, review_calibration from ..calibration_io import load_calibration from ..dataset import DatasetReport, check_dataset from ..discovery import DiscoveryResult, default_output_folder, discover_experiment @@ -222,6 +223,30 @@ def _calibrated( return replace(metadata, issues=(*kept, *checked, *review.issues)), review +def fit_calibration( + pre: Preflight, review: AxisReview +) -> tuple[Preflight, tuple[ValidationIssue, ...]]: + """Record a Z or T size only for the images that have that role (design v0.4 §3). + + Parameters + ---------- + pre + The preflight, its sizes resolved from the headers and ``calibration.csv``. + review + The axis review of the roles the images are read with. + + Returns + ------- + tuple + The preflight with those sizes removed and ``calibration_axis_absent`` among its + issues, and the new issues alone (:func:`.calibration.fit_to_roles`). + """ + roles = {path: mapping.roles for path, mapping in review.file_mappings.items()} + fitted, issues = fit_to_roles(pre.calibration, roles) + metadata = replace(pre.metadata, issues=(*pre.metadata.issues, *issues)) + return replace(pre, metadata=metadata, calibration=fitted), issues + + def _without_excluded_masks( discovery: DiscoveryResult, excluded: tuple[str, ...] ) -> DiscoveryResult: diff --git a/tests/test_absent_axis_sizes.py b/tests/test_absent_axis_sizes.py new file mode 100644 index 00000000..75832ae5 --- /dev/null +++ b/tests/test_absent_axis_sizes.py @@ -0,0 +1,144 @@ +"""PP-8c item 2: no pixel size is recorded for an axis the image does not have. + +A max projection saved as ``CYX`` (MorphoRelate's HPMEC data) keeps the z-step of its +original stack in its metadata (Z = 0.43 µm). The run recorded it as the image's Z size, +from the header, although the image has no Z. Now a Z (or T) size is recorded only for an +image with that role; otherwise the axis has no value and no source. A ``calibration.csv`` +statement for an axis an image lacks is not recorded for it either: the warning +``calibration_axis_absent`` names the images. +""" + +import json + +import numpy as np +import tifffile + +from morphobricks_preprocessing import ( + RunInputs, + prefill_mapping, + read_manifest, + run_experiment, +) +from morphobricks_preprocessing.cli import main + +HEADER = {"PhysicalSizeX": 0.5, "PhysicalSizeY": 0.5, "PhysicalSizeZ": 0.43, "TimeIncrement": 2.0} + + +def write(path, axes, shape, **metadata): + path.parent.mkdir(parents=True, exist_ok=True) + data = np.random.default_rng(0).integers(0, 4000, shape, dtype=np.uint16) + tifffile.imwrite( + path, data, ome=True, photometric="minisblack", metadata={"axes": axes, **metadata} + ) + + +def sizes(output, path): + entry = read_manifest(output / "run_manifest.json").file(path) + return {size.role: size.to_dict() for size in entry.pixel_sizes} + + +def none(role): + return {"role": role, "value": None, "unit": None, "source": None} + + +def report(output): + return json.loads((output / "discovery_report.json").read_text(encoding="utf-8")) + + +def state(output, text): + output.mkdir(parents=True, exist_ok=True) + (output / "calibration.csv").write_text("target,axis,size,unit\n" + text, encoding="utf-8") + + +def test_a_projection_records_no_z_or_t_size_from_its_header(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "a.ome.tif", "CYX", (2, 8, 8), **HEADER) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=output) + assert result.written + found = sizes(output, "ctrl/a.ome.tif") + assert found["X"]["source"] == found["Y"]["source"] == "header" + assert found["Z"] == none("Z") + assert found["T"] == none("T") + assert set(report(output)["images"]["ctrl/a.ome.tif"]["pixel_sizes"]) == {"X", "Y"} + inputs = RunInputs.open(output) + assert inputs.pixel_sizes("ctrl/a.ome.tif")["Z"].source is None + assert "calibration_axis_absent" not in {issue.code for issue in result.issues} + + +def test_an_image_with_the_role_keeps_its_header_size(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "a.ome.tif", "TZYX", (2, 3, 8, 8), **HEADER) + assert run_experiment(root, prefill_mapping("TZYX"), output_folder=output).written + found = sizes(output, "ctrl/a.ome.tif") + assert found["Z"]["value"] == 0.43 and found["Z"]["source"] == "header" + assert found["T"]["value"] == 2.0 and found["T"]["source"] == "header" + + +def test_a_z_statement_is_not_recorded_for_images_without_z(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "a.ome.tif", "YX", (8, 8), PhysicalSizeX=0.5, PhysicalSizeY=0.5) + write(root / "ctrl" / "b.ome.tif", "YX", (8, 8), PhysicalSizeX=0.5, PhysicalSizeY=0.5) + state(output, ".,Z,2,um\nctrl/a.ome.tif,X,0.4,um\n") + result = run_experiment(root, prefill_mapping("YX"), output_folder=output) + assert result.written + (absent,) = [issue for issue in result.issues if issue.code == "calibration_axis_absent"] + assert absent.severity.value == "warning" + assert absent.paths == ("ctrl/a.ome.tif", "ctrl/b.ome.tif") + assert "Z" in absent.message and "no Z" in absent.message + assert sizes(output, "ctrl/a.ome.tif")["Z"] == none("Z") + assert sizes(output, "ctrl/a.ome.tif")["X"]["source"] == "stated" + codes = [issue["code"] for issue in report(output)["issues"]] + assert "calibration_axis_absent" in codes + + +def test_a_statement_for_the_whole_experiment_sizes_only_the_images_with_the_axis(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "stack.ome.tif", "ZYX", (3, 8, 8)) + write(root / "ctrl" / "flat.ome.tif", "YX", (8, 8)) + state(output, ".,Z,2,um\n") + result = run_experiment( + root, prefill_mapping("ZYX"), output_folder=output, missing_as_singleton=True + ) + assert result.written + assert sizes(output, "ctrl/stack.ome.tif")["Z"]["source"] == "stated" + assert sizes(output, "ctrl/flat.ome.tif")["Z"] == none("Z") + (absent,) = [issue for issue in result.issues if issue.code == "calibration_axis_absent"] + assert absent.paths == ("ctrl/flat.ome.tif",) + + +def test_rows_recorded_with_a_z_size_from_an_earlier_version_are_reused(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "a.ome.tif", "CYX", (2, 8, 8), **HEADER) + assert run_experiment(root, prefill_mapping("CYX"), output_folder=output).written + table = (output / "plane_histograms.csv").read_bytes() + # The manifest as an earlier version wrote it: the unused z-step recorded. + path = output / "run_manifest.json" + data = json.loads(path.read_text(encoding="utf-8")) + data["files"][0]["pixel_sizes"][2] = { + "role": "Z", "value": 0.43, "unit": "micrometer", "source": "header" + } # fmt: skip + path.write_text(json.dumps(data), encoding="utf-8") + again = run_experiment(output_folder=output) + assert again.written and again.reused == ("ctrl/a.ome.tif",) and again.read == () + assert (output / "plane_histograms.csv").read_bytes() == table + assert sizes(output, "ctrl/a.ome.tif")["Z"] == none("Z") + + +def test_check_reports_no_z_size_for_a_projection(tmp_path, capsys): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "a.ome.tif", "CYX", (2, 8, 8), **HEADER) + assert main(["check", str(root), "--output", str(output)]) == 0 + assert set(report(output)["images"]["ctrl/a.ome.tif"]["pixel_sizes"]) == {"X", "Y"} + + +def test_the_text_report_counts_z_sizes_only_for_images_with_z(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + write(root / "ctrl" / "stack.ome.tif", "ZYX", (3, 8, 8), **HEADER) + write(root / "ctrl" / "flat.ome.tif", "YX", (8, 8), **HEADER) + result = run_experiment( + root, prefill_mapping("ZYX"), output_folder=output, missing_as_singleton=True + ) + assert result.written + text = (output / "discovery_report.txt").read_text(encoding="utf-8") + assert " Z step: 1 from the header, 0 stated, 0 in pixels, 0 unknown; 1 without Z" in text + assert "Time step" not in text # no image has T: no T size is recorded From 878e6c6a2edb4ce423f3dcffc4c526ec0ea58ae2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:03:11 +0000 Subject: [PATCH 3/9] PP-8c: read_plane and read_mask refuse a z or t that is not a whole number A plane_id passed as z gave the misleading IndexError "has no Z ..." and read_mask(z=True) reached the reader. Both now raise a TypeError naming the expected arguments; Python and NumPy integers are accepted, read_mask keeps None. No read by plane_id is added (spec: read_plane(fov, z, t)). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 3 + docs/plan.md | 9 +++ src/morphobricks_preprocessing/run_inputs.py | 18 ++++++ tests/test_absent_axis_sizes.py | 5 +- tests/test_read_arguments.py | 59 ++++++++++++++++++++ 5 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 tests/test_read_arguments.py diff --git a/docs/contract.md b/docs/contract.md index 543b5d90..08bf621a 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -233,6 +233,9 @@ It gives: as stored through the recorded `axes` and `roles` by preprocessing's reader with `check_labels`; a mask changed since the run is refused; a stated shared 2-D mask serves every plane; +- `z` and `t` are whole numbers (Python or NumPy integers; `read_mask` also takes None); any + other value, such as a `plane_id`, is a `TypeError` naming the expected arguments + (v0.4.0, plan §12 question 54). There is no read by `plane_id`: a row gives its `z` and `t`; - `histograms()` (the table as read when opened), `design()` and `image_type()`. A `RunInputs` is picklable, so worker processes can use it. diff --git a/docs/plan.md b/docs/plan.md index 467b9f2e..289fcfb7 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1184,3 +1184,12 @@ Raised during review; to be decided before the milestones that need them: images with the axis. The `RowKey` does not include pixel sizes, so earlier rows are reused (no row depends on a size, question 49): the next `restart` leaves the table byte-identical and rewrites the manifest without the absent sizes. Contract §9.3.* +54. What do `RunInputs.read_plane` and `read_mask` do with a `z` or `t` that is not a number + (PP-8c, from a run on real data)? + *Decided (2 November 2026, owner): a `TypeError` naming the expected arguments. A + `plane_id` passed as `z` gave the misleading `IndexError` "has no Z …" on a 2D image, + and `read_mask(z=True)` reached the reader. Python and NumPy integers are accepted + (`bool` is not), and `read_mask` keeps None. A read by `plane_id` is not added: the + spec's interface is `read_plane(fov, z, t)` (design v0.4 §6), every table row carries + its `z` and `t`, and a second way to address a plane would be a second rule to keep + in step. Contract §9.1.* diff --git a/src/morphobricks_preprocessing/run_inputs.py b/src/morphobricks_preprocessing/run_inputs.py index 85fafc20..733910d6 100644 --- a/src/morphobricks_preprocessing/run_inputs.py +++ b/src/morphobricks_preprocessing/run_inputs.py @@ -222,11 +222,14 @@ def read_plane( Raises ------ + TypeError + If ``z`` or ``t`` is not a whole number (a ``plane_id`` is not a coordinate). RunInputsError If the image changed since the run. IndexError If ``z`` or ``t`` is outside the image. """ + z, t = (_coordinate(v, k, "read_plane(fov, z, t)") for k, v in (("z", z), ("t", t))) entry = self._checked(fov) mapping = _mapping(entry) path = self.experiment_root / entry.relative_path @@ -268,11 +271,15 @@ def read_mask( Raises ------ + TypeError + If ``z`` or ``t`` is neither None nor a whole number. RunInputsError If the image has no recorded mask of that type, or the mask changed since the run. ValueError For a label outside 0 to the int32 limit (:func:`.masks.check_labels`). """ + call = "read_mask(fov, mask_type, z, t)" + z, t = (None if v is None else _coordinate(v, k, call) for k, v in (("z", z), ("t", t))) entry = self._checked(fov) recorded = self._mask_entry(entry.relative_path, mask_type) if not self._mask_unchanged(recorded): @@ -418,6 +425,17 @@ def __array__(self, dtype: np.dtype | None = None, copy: bool | None = None) -> return data if dtype is None else data.astype(dtype) +def _coordinate(value: object, name: str, call: str) -> int: + """Return a plane coordinate as an int, refusing anything but a whole number.""" + if isinstance(value, bool) or not isinstance(value, int | np.integer): + raise TypeError( + f"{call} takes the plane's zero-based coordinates z and t as whole numbers; " + f"{name} is {value!r} ({type(value).__name__}). A row's plane_id is not a " + "coordinate: give the row's z and t (PlaneRecord.z, PlaneRecord.t)." + ) + return int(value) + + def _manifest(output: Path) -> RunManifest: """Read an output folder's manifest, refusing runs written before v0.4.0.""" path = output / MANIFEST_FILENAME diff --git a/tests/test_absent_axis_sizes.py b/tests/test_absent_axis_sizes.py index 75832ae5..8108b115 100644 --- a/tests/test_absent_axis_sizes.py +++ b/tests/test_absent_axis_sizes.py @@ -114,9 +114,8 @@ def test_rows_recorded_with_a_z_size_from_an_earlier_version_are_reused(tmp_path # The manifest as an earlier version wrote it: the unused z-step recorded. path = output / "run_manifest.json" data = json.loads(path.read_text(encoding="utf-8")) - data["files"][0]["pixel_sizes"][2] = { - "role": "Z", "value": 0.43, "unit": "micrometer", "source": "header" - } # fmt: skip + z_step = {"role": "Z", "value": 0.43, "unit": "micrometer", "source": "header"} + data["files"][0]["pixel_sizes"][2] = z_step path.write_text(json.dumps(data), encoding="utf-8") again = run_experiment(output_folder=output) assert again.written and again.reused == ("ctrl/a.ome.tif",) and again.read == () diff --git a/tests/test_read_arguments.py b/tests/test_read_arguments.py new file mode 100644 index 00000000..3518ae5e --- /dev/null +++ b/tests/test_read_arguments.py @@ -0,0 +1,59 @@ +"""PP-8c item 5: ``RunInputs.read_plane`` and ``read_mask`` refuse coordinates of another type. + +Passing a ``plane_id`` (or any other non-integer) as ``z`` or ``t`` gave a misleading +``IndexError`` ("has no Z img.tif::z=0,t=0"), or reached the reader. Now it is a +``TypeError`` naming the expected arguments. NumPy integers are integers. +""" + +import numpy as np +import pytest + +from morphobricks_preprocessing import RunInputs +from morphobricks_preprocessing.testing import make_clean_run + + +@pytest.fixture(scope="module") +def flat(tmp_path_factory): + folder = tmp_path_factory.mktemp("flat") + clean = make_clean_run(folder, axes="YX", shape=(8, 8), mask_types=("Cells",)) + return RunInputs.open(clean.output, masks_root=clean.masks) + + +@pytest.fixture(scope="module") +def stack(tmp_path_factory): + clean = make_clean_run(tmp_path_factory.mktemp("stack"), axes="ZCYX", shape=(3, 2, 8, 8)) + return RunInputs.open(clean.output) + + +@pytest.mark.parametrize("value", ["ctrl/rep1/a.ome.tif::z=0,t=0", "0", 0.0, 1.5, True, None]) +def test_read_plane_refuses_a_z_or_t_that_is_not_an_integer(flat, value): + fov = flat.fovs()[0] + for kwargs in ({"z": value}, {"t": value}): + with pytest.raises(TypeError, match=r"read_plane\(fov, z, t\).*whole numbers"): + flat.read_plane(fov, **kwargs) + + +def test_read_plane_refuses_a_plane_id_given_as_z(stack): + fov = stack.fovs()[0] + with pytest.raises(TypeError, match="plane_id"): + stack.read_plane(fov, fov.plane_ids[0]) + + +def test_read_plane_takes_numpy_integers(stack): + fov = stack.fovs()[0] + assert np.array_equal(stack.read_plane(fov, np.int64(2)), stack.read_plane(fov, 2)) + + +@pytest.mark.parametrize("value", ["ctrl/rep1/a.ome.tif::z=0,t=0", 0.0, True]) +def test_read_mask_refuses_a_z_or_t_that_is_not_an_integer(flat, value): + fov = flat.fovs()[0] + for kwargs in ({"z": value}, {"t": value}): + with pytest.raises(TypeError, match=r"read_mask\(fov, mask_type, z, t\)"): + flat.read_mask(fov, "Cells", **kwargs) + + +def test_read_mask_still_takes_none_and_integers(flat): + fov = flat.fovs()[0] + assert np.array_equal( + flat.read_mask(fov, "Cells"), flat.read_mask(fov, "Cells", np.int32(0), 0) + ) From be205f25621e324c0f4f2291ba5192d45b0987d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:07:26 +0000 Subject: [PATCH 4/9] PP-8c: a stopped run says what it wrote Every run holding the lock writes its report, and the CLI run writes the design template when it stops on the design, yet the summary said "Nothing was written". It now says no table or manifest was written and names the report and the template when written (RunResult.report_written, run_summary(design_template=)); the notebook says the same. A run refused by the lock still says "Nothing was written." Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 1 + docs/plan.md | 8 +++ docs/user_guide.md | 3 + src/morphobricks_preprocessing/cli_run.py | 22 ++++--- src/morphobricks_preprocessing/notebook_ui.py | 2 +- src/morphobricks_preprocessing/report.py | 44 +++++++++++--- .../run/pipeline.py | 2 + src/morphobricks_preprocessing/run/result.py | 5 ++ tests/test_axis_check.py | 2 +- tests/test_cli.py | 6 +- tests/test_notebook_ui.py | 4 +- tests/test_report.py | 2 +- tests/test_stopped_run_text.py | 57 +++++++++++++++++++ 13 files changed, 135 insertions(+), 23 deletions(-) create mode 100644 tests/test_stopped_run_text.py diff --git a/docs/contract.md b/docs/contract.md index 08bf621a..eeda055e 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -361,6 +361,7 @@ apart from the limits in the manifest. | Summaries | One per image, all Z and T planes pooled per channel: median, 1st and 99.8th percentiles, saturated fraction (normalization contract §7.1). | | Warnings | `image_intensity_outlier` (an image's median or 99.8th percentile in a channel more than `outlier_limit` robust deviations from the other images of its condition and biological repeat), `repeat_intensity_shift` (a biological repeat's median of image medians far from the other repeats' images of its condition), `saturated_image` (more than `saturation_limit` of the pixels at the type's maximum), `no_intensity_spread` (1st and 99.8th percentiles equal). One warning per image or repeat, channel and compared value, naming the channel `C`, the image or `/`, the value and the group value or limit. Comparisons stay within a condition, so treatment effects are never reported. | | Limits | `IntensityCheckSettings`: `outlier_limit` 3.0, `saturation_limit` 0.01, `min_images` 5 (provisional in the normalization brick). `run_experiment(..., intensity_settings=...)` uses them and records them in the manifest (`intensity_checks`, §9); they change no row. | +| Stopped run | `RunResult.report_written` is True for every run that held the lock, also one that stops (it wrote its report); False for a run refused before (the lock, an output folder inside the experiment). A stopped run's summary (`report.run_summary`, the CLI, the notebook) says that no table or manifest was written and names what was: the report, and the design template the CLI `run` writes when it stops on the design (plan §12 question 55). | | Run | `RunResult.intensity` holds the summaries and warnings (`IntensityReport`) of a written table, None otherwise. The warnings never block and are kept apart from `RunResult.issues`. | | Notebook | Step 3 shows the checks of the table just written after every successful run, step 4 those of any chosen table: a status line with the limits, counts per kind, a table of the warnings (check, channel, image or repeat, what was found) and what to do about each kind, a collapsed **Check settings** panel (changing a limit updates the warnings at once; in step 3 the next run records it), and per channel (chosen with buttons) box plots of the image medians and high percentiles per condition and biological repeat, with images that have a warning in that channel (`Issue.channel`, N52) as red diamonds and shifted repeats as amber boxes, and the cumulative intensity distribution of each biological repeat, one panel per condition (shifted repeats dashed; at most 12 conditions). The plots are drawn in the notebook only; no file is written. | | Command line | `run` prints the warnings after the run, grouped by kind with what to do once per kind, and draws no plots; `--outlier-limit`, `--saturation-limit` (a fraction) and `--min-images` set the limits. | diff --git a/docs/plan.md b/docs/plan.md index 289fcfb7..7cf65586 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1193,3 +1193,11 @@ Raised during review; to be decided before the milestones that need them: spec's interface is `read_plane(fov, z, t)` (design v0.4 §6), every table row carries its `z` and `t`, and a second way to address a plane would be a second rule to keep in step. Contract §9.1.* +55. What does a run that stops say it wrote (PP-8c, from a run on real data)? + *Decided (2 November 2026, owner): what it wrote. Since PP-7 every run that holds the + lock writes its report, also when it stops, and the CLI `run` writes the design template + when it stops on the design, yet the summary said "Nothing was written". It now says + "The run stopped (…): no table or manifest was written." and names the report files and + the template when written (`RunResult.report_written`, `run_summary(design_template=)`); + the notebook says "The run stopped: no table or manifest was written." Only a run + refused before it starts (the lock) still says "Nothing was written."* diff --git a/docs/user_guide.md b/docs/user_guide.md index 8d723a04..4cfcc7ad 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -402,6 +402,9 @@ the design is confirmed, or why a saved one cannot be read (naming the line and never writes the file. `check` writes only the report, `discovery_report.json` and `discovery_report.txt`, to the output folder; `run` writes it on every run, also one that stops (`status` `stopped` or `written`, with the SHA-256 of the manifest it describes). +A run that stops says so, says that no table or manifest was written, and names what it +did write: the report, and the design template when it wrote one. Only a run refused before +it starts (another run holds the lock) writes nothing. A later `run` in the same output folder reuses everything the earlier run recorded: the axis roles (`--axes` is needed again only for plain stacks that are new or changed), the diff --git a/src/morphobricks_preprocessing/cli_run.py b/src/morphobricks_preprocessing/cli_run.py index 6c9a2ef5..2b08b264 100644 --- a/src/morphobricks_preprocessing/cli_run.py +++ b/src/morphobricks_preprocessing/cli_run.py @@ -247,16 +247,19 @@ def run_command(args: argparse.Namespace) -> int: intensity_settings=settings[0], histogram_settings=settings[1], ) + template, instructions = None, "" + if _stopped_on_design(result.issues, design): + template, instructions = _design_template(design, pre, args) if result.issues: print(format_issues(result.issues)) print() - print(run_summary(result)) + print(run_summary(result, design_template=template is not None)) if result.intensity is not None: print() print(intensity_text(result.intensity)) - if _stopped_on_design(result.issues, design): + if instructions: print() - print(_design_template(design, pre, args)) + print(instructions) return 0 if result.written else 1 @@ -394,21 +397,24 @@ def _stopped_on_design( return any(issue.code in found for issue in design_issues(design)) -def _design_template(design: ExperimentDesign, pre: Preflight, args: argparse.Namespace) -> str: +def _design_template( + design: ExperimentDesign, pre: Preflight, args: argparse.Namespace +) -> tuple[Path | None, str]: """Write the design for the user to fill in, holding the output folder's lock. The run, given the design, stopped on it and wrote its report; the template keeps - every answer already given. Returns what the user has to do. + every answer already given. Returns the file written (None when nothing was) and what + the user has to do. """ root = pre.discovery.root output = Path(args.output) if args.output else default_output_folder(root) if check_output_folder(root, output): - return "" # the run reported it; nothing is written inside the experiment + return None, "" # the run reported it; nothing is written inside the experiment try: with output_lock(output): target = write_experiment_design(design, output / DESIGN_FILENAME) except OutputLockedError as exc: - return format_issues([lock_issue(exc)]) + return None, format_issues([lock_issue(exc)]) lines = [ f"The experiment design was written to {target}. Open it in a spreadsheet program " "or a text editor, one row per biological-repeat folder of each condition:", @@ -424,7 +430,7 @@ def _design_template(design: ExperimentDesign, pre: Preflight, args: argparse.Na " - control: set it to true in the rows of control conditions (optional; recorded only).", "Then run again.", ] - return "\n".join(lines) + return target, "\n".join(lines) def _image_type_note(review: ImageTypeReview, given: str | None) -> str: diff --git a/src/morphobricks_preprocessing/notebook_ui.py b/src/morphobricks_preprocessing/notebook_ui.py index 8224f42a..67d5bf0b 100644 --- a/src/morphobricks_preprocessing/notebook_ui.py +++ b/src/morphobricks_preprocessing/notebook_ui.py @@ -2039,7 +2039,7 @@ def _show_run_result(self, result: RunResult) -> None: if not result.written: self.run_status.value = ui.status( "error", - "Nothing was written.", + "The run stopped: no table or manifest was written.", ["Read the messages below; each one says what to do.", "Then run again."], ) return diff --git a/src/morphobricks_preprocessing/report.py b/src/morphobricks_preprocessing/report.py index 483ebf0f..6c6867b2 100644 --- a/src/morphobricks_preprocessing/report.py +++ b/src/morphobricks_preprocessing/report.py @@ -16,8 +16,9 @@ from .axes import AxisMapping, AxisReview, axis_length_ranges, axis_name from .dataset import BalanceTable -from .design import ExperimentDesign +from .design import DESIGN_FILENAME, ExperimentDesign from .discovery import DiscoveryResult +from .discovery_report import REPORT_FILENAME, REPORT_TEXT_FILENAME from .mask_scan import DEFAULT_THRESHOLDS, MaskReport, PairRelation from .masks import MaskInventory from .run import Preflight, RunResult @@ -205,13 +206,25 @@ def preflight_summary(pre: Preflight) -> str: return "\n".join(lines) -def run_summary(result: RunResult) -> str: - """Summarise pass 2: whether output was written and what was read or kept.""" +def run_summary(result: RunResult, *, design_template: bool = False) -> str: + """Summarise pass 2: whether output was written and what was read or kept. + + Parameters + ---------- + result + What the run did. + design_template + True when ``experiment_design.csv`` was written for the user to confirm after the + run stopped on it (the CLI ``run`` does). + + Returns + ------- + str + For a stopped run, what it wrote (the report, the design template) and that no + table or manifest was written. + """ if not result.written: - return ( - f"Nothing was written ({issue_counts(result.issues)}). Resolve the errors " - "and run again." - ) + return _stopped_summary(result, design_template) lines = [ f"Wrote {plural(result.plane_count, 'plane')} to {result.csv_path}", f"Run manifest: {result.manifest_path}", @@ -237,6 +250,23 @@ def run_summary(result: RunResult) -> str: return "\n".join(lines) +def _stopped_summary(result: RunResult, design_template: bool) -> str: + """Say that a stopped run wrote no table or manifest, and what it did write.""" + written = [] + if result.report_written: + written.append(f"{REPORT_FILENAME} and {REPORT_TEXT_FILENAME} (the report)") + if design_template: + written.append(f"{DESIGN_FILENAME} (the experiment design to confirm)") + lines = [ + f"The run stopped ({issue_counts(result.issues)}): no table or manifest was written.", + f"Written to {result.output_folder}: {', '.join(written)}." + if written + else "Nothing was written.", + "Resolve the errors and run again.", + ] + return "\n".join(lines) + + @dataclass(frozen=True) class GroupSummary: """Pooled statistics of one group of planes. diff --git a/src/morphobricks_preprocessing/run/pipeline.py b/src/morphobricks_preprocessing/run/pipeline.py index d4191354..65156c1f 100644 --- a/src/morphobricks_preprocessing/run/pipeline.py +++ b/src/morphobricks_preprocessing/run/pipeline.py @@ -294,6 +294,7 @@ def _stop(run: _Run, base: RunOutcome, rows: Rows | None) -> RunResult: excluded=run.pre.excluded, mode=run.mode, input_changes=run.inputs.changes, + report_written=True, **counts(rows), ) @@ -352,6 +353,7 @@ def _publish(run: _Run, base: RunOutcome, rows: Rows, settings: RunSettings) -> mode=run.mode, replaced=tuple(sorted(outcome.replaced)), input_changes=run.inputs.changes, + report_written=True, **counts(rows), ) diff --git a/src/morphobricks_preprocessing/run/result.py b/src/morphobricks_preprocessing/run/result.py index 5af874fe..c106ce59 100644 --- a/src/morphobricks_preprocessing/run/result.py +++ b/src/morphobricks_preprocessing/run/result.py @@ -62,6 +62,10 @@ class RunResult: The earlier output files the run replaced or removed (the report says why). input_changes The inputs given that differ from those the earlier run recorded. + report_written + True when the run wrote its report (every run that holds the lock does, also when + it stops); False when it was refused before (the lock, an output folder inside + the experiment). """ output_folder: Path @@ -81,6 +85,7 @@ class RunResult: mode: str = RESTART replaced: tuple[str, ...] = () input_changes: tuple[InputChange, ...] = () + report_written: bool = False @property def ok(self) -> bool: diff --git a/tests/test_axis_check.py b/tests/test_axis_check.py index d3e37825..b59820ae 100644 --- a/tests/test_axis_check.py +++ b/tests/test_axis_check.py @@ -387,7 +387,7 @@ def test_cli_run_names_the_swapped_files(tmp_path, capsys): assert main(["run", str(root), "--axes", "CXY"]) == 1 out = capsys.readouterr().out assert "header CYX, confirmed CXY: ctrl/a.tif" in out - assert "Nothing was written" in out + assert "no table or manifest was written" in out def test_notebook_marks_roles_that_disagree_with_the_header(tmp_path): diff --git a/tests/test_cli.py b/tests/test_cli.py index e3329b4b..2450420c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -127,7 +127,7 @@ def test_rgb_is_refused_for_images_that_cannot_be_rgb(experiment, capsys): out = capsys.readouterr().out assert "contradict their header" in out assert "RGB images need exactly three colour channels" in out - assert "8 channels of type uint8" in out and "Nothing was written" in out + assert "8 channels of type uint8" in out and "no table or manifest was written" in out def test_image_type_must_be_known(experiment): @@ -204,7 +204,7 @@ def test_run_reports_blocking_errors(experiment, capsys): assert main(["run", str(experiment), "--axes", "Z-YX"]) == 1 out = capsys.readouterr().out assert "No role is chosen for the axis C (channel)" in out - assert "Nothing was written" in out + assert "no table or manifest was written" in out def test_run_without_readable_images(tmp_path, capsys): @@ -234,7 +234,7 @@ def test_different_channel_counts_stop_check_and_run(tmp_path, capsys): assert main(["run", str(root), "--axes", "ZCYX"]) == 1 out = capsys.readouterr().out assert "different numbers of channels" in out and "b.tif" in out - assert "Nothing was written" in out + assert "no table or manifest was written" in out def test_axes_are_needed_for_the_first_run_only(experiment, capsys): diff --git a/tests/test_notebook_ui.py b/tests/test_notebook_ui.py index ddcda3a8..0aa54b17 100644 --- a/tests/test_notebook_ui.py +++ b/tests/test_notebook_ui.py @@ -1279,7 +1279,7 @@ def test_run_with_errors_shows_what_to_do(experiment, monkeypatch): write_file(experiment / "ctrl/rep1/bad.tif", b"garbage") # appears after the check nb.run() assert not nb.result.written - assert "Nothing was written" in nb.run_status.value + assert "no table or manifest was written" in nb.run_status.value assert "could not be opened" in nb.run_issues.value assert nb.progress.bar_style == "danger" assert nb.run_next.value == "" @@ -1297,7 +1297,7 @@ def broken(*args, **kwargs): assert nb.run() is None assert "unexpected problem" in nb.run_status.value assert "RuntimeError: disk on fire" in nb.run_status.value - assert "Nothing was written" not in nb.run_status.value + assert "no table or manifest was written" not in nb.run_status.value assert "Traceback" in nb.details.value assert not nb.run_button.disabled diff --git a/tests/test_report.py b/tests/test_report.py index f14112f3..836cc6a4 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -119,7 +119,7 @@ def test_run_summaries(experiment): def test_run_summary_when_blocked(experiment): result = run_experiment(experiment, prefill_mapping("ZYX"), bin_count=0) - assert run_summary(result).startswith("Nothing was written (1 error") + assert run_summary(result).startswith("The run stopped (1 error") def test_group_summaries_pool_the_planes(experiment): diff --git a/tests/test_stopped_run_text.py b/tests/test_stopped_run_text.py new file mode 100644 index 00000000..3e76f485 --- /dev/null +++ b/tests/test_stopped_run_text.py @@ -0,0 +1,57 @@ +"""PP-8c item 4: a stopped run says what it wrote, not "Nothing was written". + +Every run that holds the lock writes its report, also when it stops (design v0.4 §4), and +the CLI ``run`` writes the design template when it stops on the design. The summary said +"Nothing was written". Now it names what was written and says no table or manifest was. +""" + +import numpy as np + +from morphobricks_preprocessing import prefill_mapping, run_experiment +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.report import run_summary +from morphobricks_preprocessing.run.lock import output_lock +from tiff_tree import write_data + + +def experiment(tmp_path): + root = tmp_path / "exp" + for seed, rel in enumerate(["ctrl/r1/a.tif", "drug/r1/a.tif"]): + data = np.random.default_rng(seed).integers(0, 200, (6, 8), dtype=np.uint8) + write_data(root / rel, data, "YX") + return root + + +def test_the_cli_names_the_report_and_the_design_template(tmp_path, capsys): + root, output = experiment(tmp_path), tmp_path / "out" + assert main(["run", str(root), "--output", str(output), "--axes", "YX"]) == 1 + out = capsys.readouterr().out + assert "Nothing was written" not in out + assert "The run stopped (1 error): no table or manifest was written." in out + assert "discovery_report.json and discovery_report.txt (the report)" in out + assert "experiment_design.csv (the experiment design to confirm)" in out + assert {p.name for p in output.iterdir()} == { + "discovery_report.json", + "discovery_report.txt", + "experiment_design.csv", + } + + +def test_a_stopped_api_run_records_that_its_report_was_written(tmp_path): + root, output = experiment(tmp_path), tmp_path / "out" + result = run_experiment(root, prefill_mapping("ZYX"), output_folder=output) # wrong roles + assert not result.written and result.report_written + text = run_summary(result) + assert text.startswith("The run stopped (") + assert "no table or manifest was written" in text + assert f"Written to {output}: discovery_report.json and discovery_report.txt" in text + assert "experiment_design.csv" not in text + + +def test_a_run_refused_by_the_lock_wrote_nothing(tmp_path): + root, output = experiment(tmp_path), tmp_path / "out" + output.mkdir() + with output_lock(output): + result = run_experiment(root, prefill_mapping("YX"), output_folder=output) + assert not result.written and not result.report_written + assert "Nothing was written." in run_summary(result) From f4a1d9b868c99453c2f17d86c6fd84863a9fc8fa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:10:12 +0000 Subject: [PATCH 5/9] PP-8c: move the masks command to cli_masks (mechanical, no behaviour change) _masks becomes masks_command, _THRESHOLD_HELP THRESHOLD_HELP, and the command's own options are added by add_masks_arguments; cli.py keeps the parser and the shared options. Layer: ui. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- pyproject.toml | 1 + src/morphobricks_preprocessing/cli.py | 87 +--------------- src/morphobricks_preprocessing/cli_masks.py | 106 ++++++++++++++++++++ 3 files changed, 111 insertions(+), 83 deletions(-) create mode 100644 src/morphobricks_preprocessing/cli_masks.py diff --git a/pyproject.toml b/pyproject.toml index 7ae9a65d..68007460 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -135,6 +135,7 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.cli" = "ui" "morphobricks_preprocessing.cli_run" = "ui" "morphobricks_preprocessing.cli_calibrate" = "ui" +"morphobricks_preprocessing.cli_masks" = "ui" "morphobricks_preprocessing.discovery_report" = "pure" "morphobricks_preprocessing.dataset" = "pure" "morphobricks_preprocessing.design" = "io" diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index aba8231f..be278f12 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -26,11 +26,11 @@ import logging import sys from collections.abc import Sequence -from dataclasses import replace from pathlib import Path from .axis_review import review_axes from .cli_calibrate import add_calibrate_parser, calibrate_command, calibration_summary +from .cli_masks import add_masks_arguments, masks_command from .cli_run import add_run_arguments, command_preflight, parse_size, run_command from .design import ( DESIGN_FILENAME, @@ -39,21 +39,13 @@ ) from .discovery import default_output_folder from .image_type import review_image_type -from .mask_scan import ( - DEFAULT_THRESHOLDS, - MASK_REPORT_FILENAME, - mask_thresholds, - scan_masks, - write_mask_report, -) -from .masks import MaskInventory, mask_axes_option +from .masks import mask_axes_option from .report import ( axis_check_summary, balance_summary, design_summary, format_issues, label_tree, - mask_report_summary, mask_summary, preflight_summary, ) @@ -63,9 +55,7 @@ remove_stale_lock, write_check_report, ) -from .run.lock import lock_issue, output_lock from .run.preflight import fit_calibration -from .validation import has_errors __all__ = ["build_parser", "main", "parse_size"] @@ -94,33 +84,11 @@ def build_parser() -> argparse.ArgumentParser: help="find every image's masks, describe them and how they relate (reads the masks)", ) _common(masks) - masks.add_argument("--log", metavar="FILE", help="write technical details to this file") - limits = masks.add_argument_group( - "relationship thresholds", "change the defaults (contract §15.1); shares are 0 to 1" - ) - for name, text in _THRESHOLD_HELP.items(): - limits.add_argument( - "--" + name.replace("_", "-"), - dest=name, - type=float, - metavar="X", - help=f"{text} (default {DEFAULT_THRESHOLDS[name]:g})", - ) + add_masks_arguments(masks) add_calibrate_parser(commands) return parser -_THRESHOLD_HELP = { - "inside_pixel_share": "share of a child's pixels in one parent for it to be inside", - "inside_object_share": "share of the children inside for a mask to be inside another", - "parent_larger_share": "share of the inside children whose parent is larger", - "same_objects_iou": "intersection over union for two objects to match one to one", - "same_objects_share": "share of the objects of both masks matched for the same objects", - "grouped_label_share": "share of a grouped mask's pixels carrying the parent's label", - "grouped_median_pieces": "median pieces per label above which a mask is grouped", -} - - def main(argv: Sequence[str] | None = None) -> int: """Run the command line; return the exit status.""" _safe_output() @@ -139,7 +107,7 @@ def main(argv: Sequence[str] | None = None) -> int: return 1 if args.remove_stale_lock and not _remove_stale_lock(args): return 1 - return {"check": _check, "run": run_command, "masks": _masks}[args.command](args) + return {"check": _check, "run": run_command, "masks": masks_command}[args.command](args) finally: if handler is not None: logging.getLogger("morphobricks_preprocessing").removeHandler(handler) @@ -291,53 +259,6 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> None: ) -def _masks(args: argparse.Namespace) -> int: - """Scan the masks and write ``mask_relationships.json``, holding the folder's lock.""" - changes = {name: getattr(args, name) for name in _THRESHOLD_HELP} - try: - thresholds = mask_thresholds({k: v for k, v in changes.items() if v is not None}) - except ValueError as exc: - print(exc) - return 1 - pre = command_preflight(args) - pre = replace(pre, masks=pre.masks or MaskInventory(folder=None)) # none found - print(mask_summary(pre.masks)) - blocking = [*pre.discovery.issues, *pre.exclusion_issues, *pre.masks.issues] - if blocking: - print() - print(format_issues(blocking)) - if has_errors(blocking): - return 1 - if not pre.masks.types: - print("Give the masks folder with --masks, or use OME-Zarr images with label images.") - return 1 - - def progress(done: int, total: int, path: str) -> None: - if path: - print(f"[{done + 1}/{total}] {path}", flush=True) - - 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, 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) - output.mkdir(parents=True, exist_ok=True) - try: - with output_lock(output): # a run may be replacing or removing the same file - path = write_mask_report(report, output / MASK_REPORT_FILENAME) - except OutputLockedError as exc: - print() - print(format_issues([lock_issue(exc)])) - return 1 - print() - print(mask_report_summary(report)) - print() - print(f"Report written to {path}") - return 0 - - def _attach_log(path: str) -> logging.Handler: handler = logging.FileHandler(path, encoding="utf-8") handler.setLevel(logging.DEBUG) diff --git a/src/morphobricks_preprocessing/cli_masks.py b/src/morphobricks_preprocessing/cli_masks.py new file mode 100644 index 00000000..3981cc3b --- /dev/null +++ b/src/morphobricks_preprocessing/cli_masks.py @@ -0,0 +1,106 @@ +"""The command line's ``masks``: its options and what it prints (moved from :mod:`.cli`). + +``masks`` is the optional mask step: it pairs every image with its masks (``--masks``, and +the label images of OME-Zarr images), reads them, and writes ``mask_relationships.json`` +with each mask type and the relationships between them (contract §15). +""" + +from __future__ import annotations + +import argparse +from dataclasses import replace +from pathlib import Path + +from .cli_run import command_preflight +from .discovery import default_output_folder +from .mask_scan import ( + DEFAULT_THRESHOLDS, + MASK_REPORT_FILENAME, + mask_thresholds, + scan_masks, + write_mask_report, +) +from .masks import MaskInventory +from .report import format_issues, mask_report_summary, mask_summary +from .run import OutputLockedError +from .run.lock import lock_issue, output_lock +from .validation import has_errors + +THRESHOLD_HELP = { + "inside_pixel_share": "share of a child's pixels in one parent for it to be inside", + "inside_object_share": "share of the children inside for a mask to be inside another", + "parent_larger_share": "share of the inside children whose parent is larger", + "same_objects_iou": "intersection over union for two objects to match one to one", + "same_objects_share": "share of the objects of both masks matched for the same objects", + "grouped_label_share": "share of a grouped mask's pixels carrying the parent's label", + "grouped_median_pieces": "median pieces per label above which a mask is grouped", +} + + +def add_masks_arguments(masks: argparse.ArgumentParser) -> None: + """Add the options of ``masks`` after the shared ones. + + Parameters + ---------- + masks + The ``masks`` command's parser. + """ + masks.add_argument("--log", metavar="FILE", help="write technical details to this file") + limits = masks.add_argument_group( + "relationship thresholds", "change the defaults (contract §15.1); shares are 0 to 1" + ) + for name, text in THRESHOLD_HELP.items(): + limits.add_argument( + "--" + name.replace("_", "-"), + dest=name, + type=float, + metavar="X", + help=f"{text} (default {DEFAULT_THRESHOLDS[name]:g})", + ) + + +def masks_command(args: argparse.Namespace) -> int: + """Scan the masks and write ``mask_relationships.json``, holding the folder's lock.""" + changes = {name: getattr(args, name) for name in THRESHOLD_HELP} + try: + thresholds = mask_thresholds({k: v for k, v in changes.items() if v is not None}) + except ValueError as exc: + print(exc) + return 1 + pre = command_preflight(args) + pre = replace(pre, masks=pre.masks or MaskInventory(folder=None)) # none found + print(mask_summary(pre.masks)) + blocking = [*pre.discovery.issues, *pre.exclusion_issues, *pre.masks.issues] + if blocking: + print() + print(format_issues(blocking)) + if has_errors(blocking): + return 1 + if not pre.masks.types: + print("Give the masks folder with --masks, or use OME-Zarr images with label images.") + return 1 + + def progress(done: int, total: int, path: str) -> None: + if path: + print(f"[{done + 1}/{total}] {path}", flush=True) + + 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, 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) + output.mkdir(parents=True, exist_ok=True) + try: + with output_lock(output): # a run may be replacing or removing the same file + path = write_mask_report(report, output / MASK_REPORT_FILENAME) + except OutputLockedError as exc: + print() + print(format_issues([lock_issue(exc)])) + return 1 + print() + print(mask_report_summary(report)) + print() + print(f"Report written to {path}") + return 0 From 47e00de5bc6b075edc34c82a51c87e8fc7d6ead3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:13:35 +0000 Subject: [PATCH 6/9] PP-8c: the masks command uses the recorded inputs, like run The experiment argument is optional when the output folder's v2 manifest records it (experiment_not_recorded otherwise); the masks folder, mask axes and exclusions are the recorded ones unless given (--no-exclude clears the exclusions). A value given that differs is used for the scan and named; the manifest is not changed. The recorded-experiment lookup is one helper shared with run (cli_run.use_recorded_experiment). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 1 + docs/plan.md | 11 +++ docs/user_guide.md | 13 ++- src/morphobricks_preprocessing/cli.py | 6 +- src/morphobricks_preprocessing/cli_masks.py | 46 ++++++++++- src/morphobricks_preprocessing/cli_run.py | 31 +++++-- tests/test_masks_recorded_inputs.py | 89 +++++++++++++++++++++ 7 files changed, 185 insertions(+), 12 deletions(-) create mode 100644 tests/test_masks_recorded_inputs.py diff --git a/docs/contract.md b/docs/contract.md index eeda055e..0349726a 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -251,6 +251,7 @@ the bricks' tests. | Files a run replaces or removes | Only `plane_histograms.csv`, `run_manifest.json`, `experiment_design.csv` (when the run has a design), the report files, `mask_relationships.json` and its own temporary folders; never `calibration.csv` (§9.3). `mask_relationships.json` is removed by a `full` run and by a `restart` whose masks entry (§9) differs from the earlier run's (or when no v2 manifest records it). Source images, masks and anything else are never touched. | | Recorded inputs | One output folder belongs to one experiment. An input not given is taken from the earlier v2 manifest: experiment root, axis mapping with the per-file plain-stack confirmations at their recorded fingerprints (a new or changed plain stack must be confirmed again), exclusions, masks folder and mask-axis statements (statements given are added per mask type), image type, design (`experiment_design.csv` in the output folder), bins (not across pixel types), histogram and intensity settings, `missing_as_singleton`; the stated calibration is the output folder's `calibration.csv` (§9.3). A value given that differs is used and reported as a change with its effect (rows recomputed, or none); for the design, a design given whose answers (repeat group, confirmation, control) differ for folders in both, or `experiment_design.csv` edited since the earlier run wrote it; for the calibration, `calibration.csv` added, edited or removed since the earlier run read it (`calibration`, "no row is affected; the pixel sizes of N images change"). Masks are dropped only with `masks_folder=False` / `--no-masks`; exclusions are cleared with `exclude=[]` / `--no-exclude`. A version 1 manifest gives nothing. | | Experiment root | `run_experiment(experiment_root=None, output_folder=...)` and CLI `run --output FOLDER` use the recorded experiment root; without one, `ValueError` (API) or `experiment_not_recorded` (CLI). Another root than the recorded one is a change: every row is read again, and the recorded plain-stack confirmations are not applied there (each plain stack must be confirmed again). | +| `masks` command | Uses the recorded inputs the same way (plan §12 question 56): the experiment argument is optional when `--output` holds a v2 manifest recording it (`experiment_not_recorded` otherwise), and the masks folder, mask-axis statements and exclusions are the recorded ones unless given (`--no-exclude` clears the exclusions). A value given that differs is used for that scan and printed as a change; the manifest is not changed (the next run records only what it is given). | | Axis roles | `mapping=None` uses the recorded roles; without any, the run stops with `axes_not_confirmed`. CLI `--axes` is needed for the first run and for plain stacks that are new or changed. | | Lock | `.morphobricks-preprocessing.lock` in the output folder, created with `O_CREAT \| O_EXCL`, JSON `host`, `pid`, `started_at`; held by `run`, by `check` while it writes its report and by `masks` while it writes `mask_relationships.json` (CLI `run` also holds it to write the design template after a run stopped on the design), removed on every exit, and removed when the lock file cannot be written. A second run is refused with `output_locked`, naming the holder, and writes nothing. `remove_stale_lock(output)` / `--remove-stale-lock` removes a lock only on that request, never one of a process still running on the same computer (checked on POSIX). Not written inside the experiment. The lock is not part of the manifest. | | Report | `discovery_report.json` (report version 1) and `discovery_report.txt`, written on every run that holds the lock, also when it stops: `step` (`run` or `check`), `status` (`stopped` or `written`; `check` is always `stopped`), `provisional` (true for the report written after the header checks, before any pixel is read), `manifest_sha256` (the manifest published with it, `written` only), `experiment_root` and `experiment_root_changed`, `output_folder`, `masks_folder`, `excluded`, `groups` (labels, position identifiers, image counts), `channels`, `channel_names`, `images` (per image `axes`, `roles`, `axes_from`, `pixel_sizes` by role with unit and source, `header`, `stated` or `pixels`, or `"uncalibrated"` without an X and a Y entry), `calibration` (`file`, `sha256` and the `statements`), `design`, `image_type`, `masks` (per type paired masks and images without one; unpaired and ambiguous masks), `mode`, `reused`, `recomputed` (file: reason), `removed`, `replaced` (file: what happened), `input_changes`, `issues` (code, severity, message, paths, action). For people only: bricks never read it. A published report is replaced atomically with the table and manifest, last. | diff --git a/docs/plan.md b/docs/plan.md index 7cf65586..1330afd1 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1201,3 +1201,14 @@ Raised during review; to be decided before the milestones that need them: the template when written (`RunResult.report_written`, `run_summary(design_template=)`); the notebook says "The run stopped: no table or manifest was written." Only a run refused before it starts (the lock) still says "Nothing was written."* +56. Does the `masks` command use the recorded inputs (PP-8c, from a run on real data)? + *Decided (2 November 2026, owner): yes, like `run` (question 48). The experiment + argument is optional when the output folder holds a v2 manifest recording it (else + `experiment_not_recorded`); the masks folder and mask axes are the recorded ones unless + given, and a value given that differs is named with its effect. The exclusions follow + the same rule (with `--no-exclude`), so `masks` never reads a mask of an image the run + left out unless asked. `--no-masks` is not needed. A change is used for that scan only: + `masks` writes only `mask_relationships.json`, never the manifest, so the next `run` + keeps the recorded value until it is given the new one, and then removes a + `mask_relationships.json` written for other masks (question 48). The recorded-experiment + lookup is one helper shared by both commands (`cli_run.use_recorded_experiment`).* diff --git a/docs/user_guide.md b/docs/user_guide.md index 4cfcc7ad..f91d45e6 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -463,7 +463,18 @@ mask that segment the same objects are reported as such. These are suggestions f pixels, not decisions: you name each kind of mask and check the relationships in the bricks that use them. The report is written to `mask_relationships.json` in the output folder (§6). The command takes `--output`, `--exclude` and `--log` like the others, and exits with -status 1 when an error blocks it or when no masks are found. Label 0 is the background and +status 1 when an error blocks it or when no masks are found. Like `run`, it uses what the +run in the output folder recorded: after a run, + +```bash +morphobricks-preprocessing masks --output /data/exp1_MorphoBricks_Preprocessing +``` + +scans the recorded experiment folder with the recorded masks folder, mask axes and +exclusions (`--no-exclude` clears the exclusions for this scan). Without an experiment +folder and without a run recording one, it stops with `experiment_not_recorded`. A value +you give that differs from the recorded one is used for this scan and printed as a change; +the run keeps its recorded value until you give it the new one. Label 0 is the background and objects have positive labels: a mask with a negative value stops the command with a message naming the mask, and no report is written. diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index be278f12..bfef4dfd 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -83,7 +83,7 @@ def build_parser() -> argparse.ArgumentParser: "masks", help="find every image's masks, describe them and how they relate (reads the masks)", ) - _common(masks) + _common(masks, recorded=True) add_masks_arguments(masks) add_calibrate_parser(commands) return parser @@ -117,8 +117,8 @@ def main(argv: Sequence[str] | None = None) -> int: def _common(parser: argparse.ArgumentParser, *, recorded: bool = False) -> None: """Add the experiment and the options every command shares. - With ``recorded``, the experiment may be left out: ``run`` then uses the one the run - in ``--output`` recorded (design v0.4 §1). + With ``recorded``, the experiment may be left out: ``run`` and ``masks`` then use the + one the run in ``--output`` recorded (design v0.4 §1). """ if recorded: parser.add_argument( diff --git a/src/morphobricks_preprocessing/cli_masks.py b/src/morphobricks_preprocessing/cli_masks.py index 3981cc3b..29fd1597 100644 --- a/src/morphobricks_preprocessing/cli_masks.py +++ b/src/morphobricks_preprocessing/cli_masks.py @@ -2,7 +2,10 @@ ``masks`` is the optional mask step: it pairs every image with its masks (``--masks``, and the label images of OME-Zarr images), reads them, and writes ``mask_relationships.json`` -with each mask type and the relationships between them (contract §15). +with each mask type and the relationships between them (contract §15). Like ``run``, it +takes every input it is not given from the run in the output folder (design v0.4 §1): the +experiment folder (``experiment_not_recorded`` without one), the masks folder, the mask +axes and the exclusions; a value given that differs is used and named. """ from __future__ import annotations @@ -11,7 +14,7 @@ from dataclasses import replace from pathlib import Path -from .cli_run import command_preflight +from .cli_run import command_preflight, use_recorded_experiment from .discovery import default_output_folder from .mask_scan import ( DEFAULT_THRESHOLDS, @@ -24,6 +27,8 @@ from .report import format_issues, mask_report_summary, mask_summary from .run import OutputLockedError from .run.lock import lock_issue, output_lock +from .run.recorded import GivenInputs, InputChange, resolve_inputs +from .run.reuse import read_previous from .validation import has_errors THRESHOLD_HELP = { @@ -37,6 +42,11 @@ } +_RECORDED = ("experiment_root", "exclude", "masks_folder", "mask_axes") +"""The recorded inputs ``masks`` uses.""" +_EFFECT = "used for this scan only; the run keeps the recorded value until it is given this one" + + def add_masks_arguments(masks: argparse.ArgumentParser) -> None: """Add the options of ``masks`` after the shared ones. @@ -45,6 +55,11 @@ def add_masks_arguments(masks: argparse.ArgumentParser) -> None: masks The ``masks`` command's parser. """ + masks.add_argument( + "--no-exclude", + action="store_true", + help="clear the exclusions of the earlier run for this scan (not with --exclude)", + ) masks.add_argument("--log", metavar="FILE", help="write technical details to this file") limits = masks.add_argument_group( "relationship thresholds", "change the defaults (contract §15.1); shares are 0 to 1" @@ -67,9 +82,14 @@ def masks_command(args: argparse.Namespace) -> int: except ValueError as exc: print(exc) return 1 + if not use_recorded_experiment(args): + return 1 + changes = _recorded_inputs(args) pre = command_preflight(args) pre = replace(pre, masks=pre.masks or MaskInventory(folder=None)) # none found print(mask_summary(pre.masks)) + for change in changes: + print(f"Changed {change.name}: {change.recorded} -> {change.given} ({change.effect})") blocking = [*pre.discovery.issues, *pre.exclusion_issues, *pre.masks.issues] if blocking: print() @@ -104,3 +124,25 @@ def progress(done: int, total: int, path: str) -> None: print() print(f"Report written to {path}") return 0 + + +def _recorded_inputs(args: argparse.Namespace) -> tuple[InputChange, ...]: + """Fill the masks folder, mask axes and exclusions not given from the recorded run. + + Returns the inputs given that differ from the recorded ones, with their effect. + """ + root = Path(args.experiment).resolve() + output = Path(args.output) if args.output else default_output_folder(root) + exclude = () if args.no_exclude else (tuple(args.exclude) or None) + given = GivenInputs( + experiment_root=root, + exclude=exclude, + masks_folder=Path(args.masks).resolve() if args.masks else None, + mask_axes=args.mask_axes or None, + ) + resolved = resolve_inputs(given, read_previous(output.resolve())[0]) + args.masks, args.mask_axes = resolved.masks_folder, resolved.mask_axes + args.exclude = list(resolved.exclude) + return tuple( + replace(change, effect=_EFFECT) for change in resolved.changes if change.name in _RECORDED + ) diff --git a/src/morphobricks_preprocessing/cli_run.py b/src/morphobricks_preprocessing/cli_run.py index 2b08b264..4114c895 100644 --- a/src/morphobricks_preprocessing/cli_run.py +++ b/src/morphobricks_preprocessing/cli_run.py @@ -93,6 +93,29 @@ def command_preflight(args: argparse.Namespace) -> Preflight: ) +def use_recorded_experiment(args: argparse.Namespace) -> bool: + """Take the experiment folder the run in ``--output`` recorded, when none is given. + + Parameters + ---------- + args + The command's arguments; ``args.experiment`` is set to the recorded folder. + + Returns + ------- + bool + False, after printing ``experiment_not_recorded``, when none is given or recorded. + """ + if args.experiment is not None: + return True + root, missing = recorded_experiment_root(args.output and Path(args.output).resolve()) + if missing is not None: + print(format_issues([missing])) + return False + args.experiment = str(root) + return True + + def add_run_arguments(run: argparse.ArgumentParser) -> None: """Add the options of ``run`` after the shared ones. @@ -204,12 +227,8 @@ def _run_settings(run: argparse.ArgumentParser) -> None: def run_command(args: argparse.Namespace) -> int: """Run pass 2 with the command's options, print what it did and return the status.""" - if args.experiment is None: - root, missing = recorded_experiment_root(args.output and Path(args.output).resolve()) - if missing is not None: - print(format_issues([missing])) - return 1 - args.experiment = str(root) + if not use_recorded_experiment(args): + return 1 output = ( Path(args.output) if args.output else default_output_folder(Path(args.experiment).resolve()) ) diff --git a/tests/test_masks_recorded_inputs.py b/tests/test_masks_recorded_inputs.py new file mode 100644 index 00000000..6643c0af --- /dev/null +++ b/tests/test_masks_recorded_inputs.py @@ -0,0 +1,89 @@ +"""PP-8c item 3: the ``masks`` command uses the inputs the run recorded, like ``run`` (PP-7). + +Given only ``--output``, ``masks`` takes the experiment folder from the folder's v2 +manifest (``experiment_not_recorded`` without one), and the masks folder, mask axes and +exclusions recorded there unless they are given. A value given that differs from the +recorded one is used, and named with its effect. +""" + +import json + +import numpy as np + +from morphobricks_preprocessing import default_design, prefill_mapping, preflight, run_experiment +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.dataset import biological_repeats +from morphobricks_preprocessing.testing import make_clean_run +from tiff_tree import write_data, write_plain + +RELATIONS = "mask_relationships.json" + + +def relations(output): + return json.loads((output / RELATIONS).read_text(encoding="utf-8")) + + +def test_masks_given_only_the_output_folder_uses_the_recorded_run(tmp_path, capsys): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8), mask_types=("Cells",)) + assert main(["masks", "--output", str(clean.output)]) == 0 + out = capsys.readouterr().out + assert "Changed" not in out + assert relations(clean.output) # written, from the recorded experiment and masks + + +def test_masks_takes_the_recorded_masks_folder(tmp_path, capsys): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8), mask_types=("Cells",)) + assert main(["masks", str(clean.experiment), "--output", str(clean.output)]) == 0 + assert (clean.output / RELATIONS).is_file() + + +def test_masks_without_an_experiment_or_a_recorded_run_stops(tmp_path, capsys): + assert main(["masks", "--output", str(tmp_path / "out")]) == 1 + out = capsys.readouterr().out + assert "No experiment folder was given" in out + assert not (tmp_path / "out" / RELATIONS).exists() + + +def test_masks_names_a_masks_folder_that_differs_from_the_recorded_one(tmp_path, capsys): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8), mask_types=("Cells",)) + other = tmp_path / "other_masks" + (clean.masks / "Cells").rename(clean.masks / "Nuclei") + clean.masks.rename(other) + assert main(["masks", "--output", str(clean.output), "--masks", str(other)]) == 0 + out = capsys.readouterr().out + assert f"Changed masks_folder: {clean.masks.as_posix()} -> {other.as_posix()}" in out + assert "the run keeps the recorded value until it is given this one" in out + + +def test_masks_takes_the_recorded_mask_axes_of_plain_stacks(tmp_path, capsys): + root, masks, output = tmp_path / "exp", tmp_path / "masks", tmp_path / "out" + for seed, rel in enumerate(["ctrl/r1/a.tif", "drug/r1/a.tif"]): + write_data(root / rel, np.random.default_rng(seed).integers(0, 9, (3, 8, 8), "u1"), "ZYX") + labels = np.zeros((3, 8, 8), np.uint16) + labels[:, 1:4, 1:4] = 1 + write_plain(masks / "Cells" / rel, labels) # a plain stack: its axes are stated + design = default_design(biological_repeats(preflight(root).discovery)) + design = design.with_confirmed("r1", True, "2026-11-02") + result = run_experiment( + root, + prefill_mapping("ZYX"), + output_folder=output, + masks_folder=masks, + mask_axes={"Cells": "ZYX"}, + design=design, + ) + assert result.written, result.issues + assert main(["masks", "--output", str(output)]) == 0 + assert (output / RELATIONS).is_file() + + +def test_masks_takes_the_recorded_exclusions(tmp_path, capsys): + clean = make_clean_run(tmp_path, axes="YX", shape=(8, 8), mask_types=("Cells",)) + excluded = "ctrl/rep1/b.ome.tif" + assert run_experiment(output_folder=clean.output, exclude=[excluded]).written + assert main(["masks", "--output", str(clean.output)]) == 0 + out = capsys.readouterr().out + assert "[3/3]" in out and "/4]" not in out # the excluded image's mask is not read + assert "Changed" not in out + assert main(["masks", "--output", str(clean.output), "--no-exclude"]) == 0 + assert "Changed exclude: ctrl/rep1/b.ome.tif -> none" in capsys.readouterr().out From fad1f75b4b27ee6b7011d930e0eed750641e4a94 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:24:16 +0000 Subject: [PATCH 7/9] PP-8c: check writes the design template; new design command check also writes experiment_design.csv prefilled from the folders when the output folder holds none (under the lock, folders without errors; never over a design). design --output OUT prints the design and what is still unconfirmed, and changes it without editing the file (--repeat-group, --control, --no-control, --confirm, --confirm-all) under the lock with the run's reader and validator; unknown names and groups breaking the rules are refused with nothing changed. Python: edit_design with DesignEdit (apply_design_edit without files). Nothing is confirmed by default. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- README.md | 20 ++ docs/contract.md | 4 +- docs/plan.md | 19 ++ docs/user_guide.md | 49 ++++- pyproject.toml | 3 + src/morphobricks_preprocessing/__init__.py | 6 + src/morphobricks_preprocessing/cli.py | 28 ++- src/morphobricks_preprocessing/cli_design.py | 139 ++++++++++++++ src/morphobricks_preprocessing/cli_run.py | 17 +- src/morphobricks_preprocessing/design_edit.py | 172 ++++++++++++++++++ src/morphobricks_preprocessing/report.py | 48 +++++ .../run/__init__.py | 7 +- .../run/check_step.py | 82 ++++++++- .../run/design_file.py | 99 ++++++++++ tests/run_files.py | 6 + tests/test_cli.py | 8 +- tests/test_design_command.py | 142 +++++++++++++++ tests/test_modes_lock_report.py | 6 +- 18 files changed, 809 insertions(+), 46 deletions(-) create mode 100644 src/morphobricks_preprocessing/cli_design.py create mode 100644 src/morphobricks_preprocessing/design_edit.py create mode 100644 src/morphobricks_preprocessing/run/design_file.py create mode 100644 tests/test_design_command.py diff --git a/README.md b/README.md index 1feaa23e..49fe647a 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,26 @@ with `read_experiment_design`. They also ask for the image type, `fluorescence` `image_type=`. After writing, the run checks the images' intensities from the table (`result.intensity`); `check_table(path)` checks any table. +## Command line + +```bash +morphobricks-preprocessing check /data/exp1 # report, suggested --axes, design template +morphobricks-preprocessing design --output /data/exp1_MorphoBricks_Preprocessing --confirm-all +morphobricks-preprocessing run /data/exp1 --axes ZCYX +morphobricks-preprocessing run --output /data/exp1_MorphoBricks_Preprocessing # later runs +morphobricks-preprocessing masks --output /data/exp1_MorphoBricks_Preprocessing # mask step +morphobricks-preprocessing calibrate --output /data/exp1_MorphoBricks_Preprocessing +``` + +`check` writes the report and, when the output folder holds no design, an +`experiment_design.csv` prefilled from the folders. `design` prints it and confirms repeat +groups (`--confirm GROUP`, `--confirm-all`), marks controls (`--control`, `--no-control`) +and pairs repeats (`--repeat-group CONDITION/REPEAT=GROUP`) without editing the file; +nothing is confirmed unless asked. `run` and `masks` reuse every input the run in the +output folder recorded. A run that stops writes its report and says so. Every output file +gets the permissions a file you save yourself would (your umask). Details in the +[user guide](docs/user_guide.md) §5. + ## Example ```python diff --git a/docs/contract.md b/docs/contract.md index 0349726a..a4b2430e 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -255,7 +255,7 @@ the bricks' tests. | Axis roles | `mapping=None` uses the recorded roles; without any, the run stops with `axes_not_confirmed`. CLI `--axes` is needed for the first run and for plain stacks that are new or changed. | | Lock | `.morphobricks-preprocessing.lock` in the output folder, created with `O_CREAT \| O_EXCL`, JSON `host`, `pid`, `started_at`; held by `run`, by `check` while it writes its report and by `masks` while it writes `mask_relationships.json` (CLI `run` also holds it to write the design template after a run stopped on the design), removed on every exit, and removed when the lock file cannot be written. A second run is refused with `output_locked`, naming the holder, and writes nothing. `remove_stale_lock(output)` / `--remove-stale-lock` removes a lock only on that request, never one of a process still running on the same computer (checked on POSIX). Not written inside the experiment. The lock is not part of the manifest. | | Report | `discovery_report.json` (report version 1) and `discovery_report.txt`, written on every run that holds the lock, also when it stops: `step` (`run` or `check`), `status` (`stopped` or `written`; `check` is always `stopped`), `provisional` (true for the report written after the header checks, before any pixel is read), `manifest_sha256` (the manifest published with it, `written` only), `experiment_root` and `experiment_root_changed`, `output_folder`, `masks_folder`, `excluded`, `groups` (labels, position identifiers, image counts), `channels`, `channel_names`, `images` (per image `axes`, `roles`, `axes_from`, `pixel_sizes` by role with unit and source, `header`, `stated` or `pixels`, or `"uncalibrated"` without an X and a Y entry), `calibration` (`file`, `sha256` and the `statements`), `design`, `image_type`, `masks` (per type paired masks and images without one; unpaired and ambiguous masks), `mode`, `reused`, `recomputed` (file: reason), `removed`, `replaced` (file: what happened), `input_changes`, `issues` (code, severity, message, paths, action). For people only: bricks never read it. A published report is replaced atomically with the table and manifest, last. | -| `check` | The same step stopped after the report; it writes only the report. | +| `check` | The same step stopped after the report; it writes the report, and the design template when the output folder holds no design (§12; owner decision of 2 November 2026, plan §12 question 57, which amends design v0.4 §1 "it writes only the report"). | ### 9.3 Calibration (`calibration.csv`, v0.4.0, design v0.4 §3) @@ -332,6 +332,8 @@ folder. | Reading | `read_experiment_design` reads labels as text; `control` and `confirmed` accept `true`/`false` in any case, also `yes`/`no` and `1`/`0`, and empty as false; a byte-order mark is ignored. The user's edits are checked (owner decision of 31 October 2026): `repeat_group` follows the identity rules of §16 (D-e: not empty, `NA`, `nan`, `none`, `null`, `n/a` or `` in any case, no leading or trailing whitespace, and spelled one way: two groups differing only in case are refused) and `confirmed_on` is empty or a date `YYYY-MM-DD`; a bad edit raises `DesignFormatError` naming the line and column. `design_issues` gives the error `invalid_experiment_design` for such values in a design built in code, so a run stops before writing, and `write_experiment_design` refuses to write them. A missing column, an invalid value, a folder listed twice, rows of one condition disagreeing on `control` or another `schema_version` raise `DesignFormatError`. `ExperimentDesign.repeat_groups()` gives the `{(condition, biological_repeat): repeat_group}` mapping the normalization brick takes. This lenient reader is for the user's own edits; other bricks read the file with the strict `read_confirmed_design` (§16). `load_design(output_folder, repeats)` reads the file with it and brings it up to date with the folders (Reruns), returning `(design, changes)`, the default design when there is no file; a file that cannot be read raises `DesignCheckError` (`unreadable_experiment_design`, §16) and is left as it is (owner decision of 31 October 2026, plan §12 question 43). | | Notebook | Step 1 asks once the labels are confirmed and writes the file after every answer (unconfirmed groups with `confirmed=false`). A design confirmed for the same folders is loaded with the questions folded away behind **Change the experiment design**. A saved file that cannot be read (`load_design` raises `DesignCheckError`, `unreadable_experiment_design`, e.g. for a `confirmed_on` a spreadsheet program rewrote as `26/10/2026`) is refused and left unchanged (owner decision of 31 October 2026, plan §12 question 42): step 1 shows the refusal, which names the line and column, starts the questions unanswered and writes nothing until the user corrects the file and confirms the labels again, or answers a question, which replaces the file. Nothing falls back to a default design silently, and step 3 does not run until every group is confirmed again. Step 3 runs only once every group is confirmed. A brick embedding step 1 with `folder_step(show_design=False)` (plan §12 question 31) gets no design: nothing is asked or written, and step 3 runs without one, as `run_experiment(design=None)`. | | Command line | `run` reads the file from the output folder, or `--design FILE`. Without a fully confirmed design it writes the design brought up to date with the folders (a template with `confirmed=false` for the groups still to confirm, keeping the answers already given) to the output folder, explains the columns, and stops with status 1; the run stops on `design_not_confirmed` first and writes its report (§9.2). Confirmed rows without a date get the date of the run. | +| Template from `check` (v0.4.0, plan §12 question 57) | `check` (CLI; `run.write_check_outputs` in Python) also writes the design prefilled from the folders, under the output folder's lock: one row per condition and biological-repeat folder, `repeat_group` the folder name, the current counts, `confirmed=false`, `control=false`. Only when the output folder holds no `experiment_design.csv` (a design there, edited or unreadable, is never replaced) and the folders have no error. | +| `design` command (v0.4.0, plan §12 question 57) | `design --output FOLDER` prints the design as a table and the repeat groups not confirmed yet, and any `repeat_group_conflict` or `invalid_experiment_design`. Options change it without editing the file, in this order: `--repeat-group CONDITION/REPEAT=GROUP` (repeatable; moves a folder, and the groups it leaves and joins are no longer confirmed, as `ExperimentDesign.with_group`), `--control CONDITION` and `--no-control CONDITION` (repeatable), `--confirm GROUP` (repeatable) and `--confirm-all` (every group not confirmed yet). It reads, changes and writes the file under the lock (`output_locked` while a run holds it), with the run's reader (`read_experiment_design`) and validator (`design_issues`); a confirmation fills `confirmed_on` with today's date and the counts are those in the file; a group already confirmed keeps its date. An unknown folder, condition or group, a condition both marked and unmarked, a group to confirm whose rows break the rules (`repeat_group_conflict`, `invalid_experiment_design`) or a design that cannot be written is refused, and nothing is changed (exit status 1). Nothing is confirmed by default. Python: `edit_design(output_folder, DesignEdit(...), on=None)` returns the design and the file written (None when nothing changed), raising `DesignEditError`; `apply_design_edit(design, edit, on)` is the same change without files. The run reconciles the design with the folders as before, so a group that gained a folder since is asked again. | | Run | `run_experiment(..., design=...)` brings the design up to date with the folders and stops with `design_not_confirmed` or `repeat_group_conflict` if it is not fully confirmed. Otherwise the file is written with the table and published before the manifest, which records its SHA-256 (§9). Without a design the run is unchanged: no file is written, an existing one is left as it is, and `experiment_design_sha256` is null. `plane_histograms.csv` does not depend on the design. | ## 13. Image type diff --git a/docs/plan.md b/docs/plan.md index 1330afd1..78dc2c0d 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1212,3 +1212,22 @@ Raised during review; to be decided before the milestones that need them: keeps the recorded value until it is given the new one, and then removes a `mask_relationships.json` written for other masks (question 48). The recorded-experiment lookup is one helper shared by both commands (`cli_run.use_recorded_experiment`).* +57. Where does the experiment design template come from, and can it be confirmed without + editing the CSV (PP-8c, from a run on real data)? + *Decided (2 November 2026, owner): earlier, and from the command line. `check` also + writes `experiment_design.csv` prefilled from the folders (conditions, biological + repeats, `repeat_group` the folder name, counts; `confirmed=false`, `control=false`) + when the output folder holds none, under the lock, and only when the folders have no + error; it never replaces a design, edited or unreadable. This amends design v0.4 §1 + ("`check` … writes only the report"). A new command `design --output OUT` prints the + design and what is still unconfirmed, and changes it without editing the file: + `--repeat-group CONDITION/REPEAT=GROUP`, `--control`/`--no-control CONDITION`, + `--confirm GROUP` and `--confirm-all` (a separate flag rather than `--confirm all`, so a + repeat group named `all` stays confirmable). It reads, changes and writes under the lock + with the run's reader (`read_experiment_design`) and validator (`design_issues`), fills + `confirmed_on`, refuses unknown names and a group whose rows break the rules, and + changes nothing when it refuses. It does not rediscover the folders: it works on the + rows `check` or `run` wrote, and the run reconciles them with the folders as before, so + a group that gained a folder since is asked again. Python: `edit_design` with + `DesignEdit` (`apply_design_edit` without files). The confirmation stays an explicit + act of the user: nothing confirms by default. Contract §12.* diff --git a/docs/user_guide.md b/docs/user_guide.md index f91d45e6..b4bb1ab8 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -393,14 +393,31 @@ for images that are not three channels of a whole-number pixel type stops the ru (`rgb_not_possible`). `run` also needs the experiment design (§4, step 1). It reads `experiment_design.csv` from -the output folder (or the file given with `--design`). The first time, or when folders were -added, it writes that file and stops, so you can fill it in with a spreadsheet program or a -text editor: give rows the same `repeat_group` to pair biological repeats named differently, -set `confirmed` to `true` in every row of a repeat group whose images are comparable, and -optionally `control` to `true` for control conditions. Then run again. `check` says whether -the design is confirmed, or why a saved one cannot be read (naming the line and column); it -never writes the file. `check` writes only the report, `discovery_report.json` and -`discovery_report.txt`, to the output folder; `run` writes it on every run, also one that +the output folder (or the file given with `--design`). `check` writes that file, prefilled +from the folders, when the output folder holds none (one row per biological-repeat folder of +each condition, the repeat group named after the folder, nothing confirmed, no control); it +never replaces a design that is there. `run` does the same the first time, or when folders +were added, and stops. Fill it in with a spreadsheet program or a text editor: give rows the +same `repeat_group` to pair biological repeats named differently, set `confirmed` to `true` +in every row of a repeat group whose images are comparable, and optionally `control` to +`true` for control conditions. Or use the `design` command, which changes the file for you: + +```bash +morphobricks-preprocessing design --output /data/exp1_MorphoBricks_Preprocessing +morphobricks-preprocessing design --output /data/exp1_MorphoBricks_Preprocessing \ + --repeat-group drug/d2=day2 --control ctrl --confirm day1 --confirm day2 +``` + +The first prints the design as a table and the repeat groups not confirmed yet; nothing is +confirmed unless you ask. The second moves the folder `drug/d2` into the repeat group +`day2` (pairing it with the other conditions' `day2`), marks `ctrl` as a control and +confirms the two groups, filling in today's date. `--confirm-all` confirms every group not +confirmed yet, and `--no-control` unmarks a control. A group whose folders break the rules +(two folders of one condition, a name such as `NA`) is not confirmed, an unknown name is +refused, and the file is then left as it was. Then run again. `check` also says whether the +design is confirmed, or why a saved one cannot be read (naming the line and column). Both +commands write the report, `discovery_report.json` and `discovery_report.txt`, to the +output folder; `run` writes it on every run, also one that stops (`status` `stopped` or `written`, with the SHA-256 of the manifest it describes). A run that stops says so, says that no table or manifest was written, and names what it did write: the report, and the design template when it wrote one. Only a run refused before @@ -752,6 +769,22 @@ result = run_experiment("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/data/exp1", mapping, design=design) pairing = design.repeat_groups() # {(condition, biological_repeat): repeat_group} ``` +The `design` command's Python equivalent changes `experiment_design.csv` in the output +folder under its lock (refusing what the command refuses, with `DesignEditError`): + +```python +from morphobricks_preprocessing import DesignEdit, edit_design + +design, written = edit_design( + "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/data/exp1_MorphoBricks_Preprocessing", + DesignEdit( + repeat_groups={("treated", "rep2_day2"): "rep2"}, + control=("ctrl",), + confirm=("rep1", "rep2"), + ), +) # written is None when nothing changed +``` + Later bricks read the file with `read_experiment_design(path)`. Stated pixel sizes (§5) are built with `calibration_entry` and written with diff --git a/pyproject.toml b/pyproject.toml index 68007460..89ca1e57 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -136,9 +136,11 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.cli_run" = "ui" "morphobricks_preprocessing.cli_calibrate" = "ui" "morphobricks_preprocessing.cli_masks" = "ui" +"morphobricks_preprocessing.cli_design" = "ui" "morphobricks_preprocessing.discovery_report" = "pure" "morphobricks_preprocessing.dataset" = "pure" "morphobricks_preprocessing.design" = "io" +"morphobricks_preprocessing.design_edit" = "pure" "morphobricks_preprocessing.discovery" = "io" "morphobricks_preprocessing.image_type" = "pure" "morphobricks_preprocessing.intensity" = "io" @@ -174,6 +176,7 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.run.pipeline" = "orchestration" "morphobricks_preprocessing.run.entries" = "io" "morphobricks_preprocessing.run.check_step" = "orchestration" +"morphobricks_preprocessing.run.design_file" = "io" "morphobricks_preprocessing.run.lock" = "io" "morphobricks_preprocessing.run.modes" = "pure" "morphobricks_preprocessing.run.recorded" = "pure" diff --git a/src/morphobricks_preprocessing/__init__.py b/src/morphobricks_preprocessing/__init__.py index 2f60756b..ad87cd36 100644 --- a/src/morphobricks_preprocessing/__init__.py +++ b/src/morphobricks_preprocessing/__init__.py @@ -67,6 +67,7 @@ reconcile_design, write_experiment_design, ) +from .design_edit import DesignEdit, DesignEditError, apply_design_edit from .discovery import ( FOV_KEY, MISSING_TEXT, @@ -187,6 +188,7 @@ Preflight, RunResult, SamplePlane, + edit_design, preflight, run_experiment, sample_planes, @@ -252,6 +254,8 @@ "DatasetReport", "DesignChanges", "DesignCheckError", + "DesignEdit", + "DesignEditError", "DesignFormatError", "DesignRow", "DiscoveryResult", @@ -297,6 +301,7 @@ "ValidationIssue", "XycPlane", "ZarrUnavailableError", + "apply_design_edit", "axis_disagreements", "axis_length_ranges", "balance_table", @@ -320,6 +325,7 @@ "default_output_folder", "default_repeat_groups", "design_issues", + "edit_design", "discover_experiment", "file_mapping_for", "file_sha256", diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index bfef4dfd..8673ee00 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -1,8 +1,10 @@ -"""Command-line interface: ``morphobricks-preprocessing check``, ``run``, ``masks``, ``calibrate``. +"""Command-line interface: ``check``, ``run``, ``masks``, ``calibrate`` and ``design``. ``check`` runs the preflight (folders and image headers, no pixels) and prints -what will be analysed, the problems found and the suggested axis roles; it writes only -the report (``discovery_report.json`` and ``.txt``) to the output folder. +what will be analysed, the problems found and the suggested axis roles; it writes the +report (``discovery_report.json`` and ``.txt``) to the output folder, and the experiment +design template, prefilled from the folders, when the folder holds no design. +``design`` (:mod:`.cli_design`) shows, checks and changes that design without editing it. ``run`` (:mod:`.cli_run`) processes the experiment with the axis roles given in ``--axes``, or those of the earlier run in the same output folder; roles are never taken from metadata without being stated. It also needs the @@ -30,6 +32,7 @@ from .axis_review import review_axes from .cli_calibrate import add_calibrate_parser, calibrate_command, calibration_summary +from .cli_design import add_design_parser, design_command, design_instructions from .cli_masks import add_masks_arguments, masks_command from .cli_run import add_run_arguments, command_preflight, parse_size, run_command from .design import ( @@ -53,7 +56,7 @@ OutputLockedError, Preflight, remove_stale_lock, - write_check_report, + write_check_outputs, ) from .run.preflight import fit_calibration @@ -86,6 +89,7 @@ def build_parser() -> argparse.ArgumentParser: _common(masks, recorded=True) add_masks_arguments(masks) add_calibrate_parser(commands) + add_design_parser(commands) return parser @@ -96,6 +100,8 @@ def main(argv: Sequence[str] | None = None) -> int: args = parser.parse_args(argv) if args.command == "calibrate": return calibrate_command(args) + if args.command == "design": + return design_command(args) if getattr(args, "no_exclude", False) and args.exclude: parser.error("--exclude and --no-exclude cannot be given together") handler = _attach_log(args.log) if getattr(args, "log", None) else None @@ -231,19 +237,23 @@ def _check(args: argparse.Namespace) -> int: f"Suggested image type: {type_review.suggested}, because {type_review.reason}. " "'run' uses it unless you give --image-type." ) - path, refusal = write_check_report(pre, args.output, review) + written = write_check_outputs(pre, args.output, review) + if written.design is not None: + print() + print("\n".join(design_instructions(written.design, written.design.parent))) print() + refusal, path = written.refusal, written.report print(format_issues([refusal]) if refusal else f"Report written to {path}" if path else "") return 0 if pre.ok and not axis_errors and refusal is None else 1 def _print_design(args: argparse.Namespace, pre: Preflight) -> None: - """Print the experiment design in the output folder; ``check`` never writes it.""" + """Print the experiment design in the output folder; ``check`` never replaces it.""" output = Path(args.output) if args.output else default_output_folder(pre.discovery.root) try: design, _ = load_design(output, pre.dataset.repeats) except DesignCheckError as exc: - # Refused and left as it is: 'check' never writes the design. + # Refused and left as it is: 'check' never replaces a design. design, refusal = None, exc.issues[0] print() if design is None: @@ -254,8 +264,8 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> None: print(design_summary(design)) else: print( - "Experiment design: not confirmed yet. 'run' writes experiment_design.csv to " - "the output folder for you to confirm." + "Experiment design: not confirmed yet. 'check' writes experiment_design.csv, " + "prefilled from the folders, to the output folder for you to confirm." ) diff --git a/src/morphobricks_preprocessing/cli_design.py b/src/morphobricks_preprocessing/cli_design.py new file mode 100644 index 00000000..80cc7cb3 --- /dev/null +++ b/src/morphobricks_preprocessing/cli_design.py @@ -0,0 +1,139 @@ +"""The command line's ``design``: show, check and change the experiment design. + +``design --output FOLDER`` prints the ``experiment_design.csv`` that ``check`` or ``run`` +wrote to the output folder, as a table, with the repeat groups still to confirm and any +problem. Its options change it without editing the file: ``--repeat-group +CONDITION/REPEAT=GROUP`` moves a folder into another repeat group, ``--control`` and +``--no-control`` mark conditions, and ``--confirm GROUP`` or ``--confirm-all`` confirm +repeat groups (:func:`.run.design_file.edit_design`, under the output folder's lock). +Nothing is confirmed unless asked. +""" + +from __future__ import annotations + +import argparse +from pathlib import Path + +from .design import DESIGN_FILENAME, DesignFormatError, design_issues +from .design_edit import DesignEdit, DesignEditError, parse_repeat_group +from .report import design_table, format_issues +from .run import OutputLockedError +from .run.design_file import edit_design +from .run.lock import lock_issue + + +def add_design_parser(commands: argparse._SubParsersAction) -> None: + """Add the ``design`` command and its options. + + Parameters + ---------- + commands + The command line's subcommands. + """ + design = commands.add_parser( + "design", + help="show and check the experiment design in the output folder; confirm repeat " + "groups, mark controls and pair repeats without editing the file", + ) + design.add_argument( + "--output", + metavar="FOLDER", + required=True, + help="the output folder holding experiment_design.csv (written by check or run)", + ) + design.add_argument( + "--repeat-group", + action="append", + default=[], + metavar="CONDITION/REPEAT=GROUP", + help="move a biological-repeat folder into a repeat group, to pair repeats named " + "differently (repeatable; REPEAT is single without repeat folders); the groups it " + "leaves and joins must be confirmed again", + ) + design.add_argument( + "--control", action="append", default=[], metavar="CONDITION", help="mark a control" + ) + design.add_argument( + "--no-control", + action="append", + default=[], + metavar="CONDITION", + help="unmark a control (repeatable)", + ) + design.add_argument( + "--confirm", + action="append", + default=[], + metavar="GROUP", + help="confirm that the repeat group's images were prepared and acquired under the " + "same circumstances, so they are comparable (repeatable)", + ) + design.add_argument( + "--confirm-all", + action="store_true", + help="confirm every repeat group not confirmed yet", + ) + + +def design_command(args: argparse.Namespace) -> int: + """Change the design as asked, print it and return the exit status.""" + try: + moves = [parse_repeat_group(text) for text in args.repeat_group] + if len({key for key, _ in moves}) != len(moves): + raise DesignEditError("--repeat-group names a folder more than once") + edit = DesignEdit( + dict(moves), + tuple(args.control), + tuple(args.no_control), + tuple(args.confirm), + args.confirm_all, + ) + design, written = edit_design(args.output, edit) + except (DesignEditError, DesignFormatError) as exc: + print(f"The experiment design was not changed: {exc}") + return 1 + except OutputLockedError as exc: + print(format_issues([lock_issue(exc)])) + return 1 + path = Path(args.output).resolve() / DESIGN_FILENAME + print(f"Experiment design ({path}{', updated' if written else ''}):") + print(design_table(design)) + problems = [issue for issue in design_issues(design) if issue.code != "design_not_confirmed"] + if problems: + print() + print(format_issues(problems)) + return 0 + + +def design_instructions(path: Path, output: Path) -> list[str]: + """Say how to fill in a design template: edit the file, or use ``design``. + + Parameters + ---------- + path + The template written. + output + Its output folder. + + Returns + ------- + list of str + The lines to print. + """ + return [ + f"The experiment design was written to {path}. Open it in a spreadsheet program " + "or a text editor, one row per biological-repeat folder of each condition:", + " - repeat_group: rows with the same value are the same biological repeat across " + "conditions (by default the folder name). Change it to pair repeats whose folders " + "are named differently; each group may hold one folder per condition. It must not " + "be empty or NA.", + " - confirmed_on: the date of the confirmation as YYYY-MM-DD, or empty (the run " + "fills it in).", + " - confirmed: set it to true in every row of a repeat group once you are sure its " + "images were prepared and acquired under the same circumstances, so they are " + "comparable.", + " - control: set it to true in the rows of control conditions (optional; recorded only).", + "Or, without editing the file: morphobricks-preprocessing design --output " + f'"{output}" --confirm GROUP (or --confirm-all), --control CONDITION, ' + "--repeat-group CONDITION/REPEAT=GROUP.", + ] diff --git a/src/morphobricks_preprocessing/cli_run.py b/src/morphobricks_preprocessing/cli_run.py index 4114c895..b9eb4c49 100644 --- a/src/morphobricks_preprocessing/cli_run.py +++ b/src/morphobricks_preprocessing/cli_run.py @@ -26,6 +26,7 @@ from .axes import AxisMapping, reference_axes from .axis_review import review_axes +from .cli_design import design_instructions from .design import ( DESIGN_FILENAME, DesignFormatError, @@ -434,21 +435,7 @@ def _design_template( target = write_experiment_design(design, output / DESIGN_FILENAME) except OutputLockedError as exc: return None, format_issues([lock_issue(exc)]) - lines = [ - f"The experiment design was written to {target}. Open it in a spreadsheet program " - "or a text editor, one row per biological-repeat folder of each condition:", - " - repeat_group: rows with the same value are the same biological repeat across " - "conditions (by default the folder name). Change it to pair repeats whose folders " - "are named differently; each group may hold one folder per condition. It must not " - "be empty or NA.", - " - confirmed_on: the date of the confirmation as YYYY-MM-DD, or empty (the run " - "fills it in).", - " - confirmed: set it to true in every row of a repeat group once you are sure its " - "images were prepared and acquired under the same circumstances, so they are " - "comparable.", - " - control: set it to true in the rows of control conditions (optional; recorded only).", - "Then run again.", - ] + lines = [*design_instructions(target, output), "Then run again."] return target, "\n".join(lines) diff --git a/src/morphobricks_preprocessing/design_edit.py b/src/morphobricks_preprocessing/design_edit.py new file mode 100644 index 00000000..510e8cb7 --- /dev/null +++ b/src/morphobricks_preprocessing/design_edit.py @@ -0,0 +1,172 @@ +"""Change an experiment design without editing the CSV: groups, controls, confirmations. + +The ``design`` command and :func:`.run.design_file.edit_design` apply a :class:`DesignEdit` to +the design in ``experiment_design.csv`` (contract §12): move folders to other repeat +groups, mark conditions as controls or not, and confirm repeat groups. Each change uses the +design's own rules (:class:`.design.ExperimentDesign`), and a repeat group is confirmed only +when its rows pass the run's validator (:func:`.design.design_issues`). Nothing is confirmed +by default: the confirmation stays an explicit act of the user. Anything unknown is refused +with nothing changed. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Mapping +from dataclasses import dataclass, field +from types import MappingProxyType + +from .design import ExperimentDesign, design_issues + +_UNCONFIRMED = "design_not_confirmed" +"""The one design issue a repeat group about to be confirmed may have.""" + + +class DesignEditError(ValueError): + """Raised when a change cannot be made to the design; the design is left as it is.""" + + +@dataclass(frozen=True) +class DesignEdit: + """The changes to make to an experiment design, applied in this order. + + Attributes + ---------- + repeat_groups + ``{(condition, biological_repeat): repeat group}``: move each folder into that + group; both groups it leaves and joins are no longer confirmed. + control, not_control + Conditions to mark as controls, and to unmark. + confirm + Repeat groups to confirm (after the moves). + confirm_all + Confirm every repeat group not confirmed yet. + """ + + repeat_groups: Mapping[tuple[str, str], str] = field(default_factory=dict) + control: tuple[str, ...] = () + not_control: tuple[str, ...] = () + confirm: tuple[str, ...] = () + confirm_all: bool = False + + def __post_init__(self) -> None: + object.__setattr__(self, "repeat_groups", MappingProxyType(dict(self.repeat_groups))) + for name in ("control", "not_control", "confirm"): + object.__setattr__(self, name, tuple(getattr(self, name))) + + +def parse_repeat_group(text: str) -> tuple[tuple[str, str], str]: + """Read ``CONDITION/REPEAT=GROUP``, the command line's way to move a folder. + + Parameters + ---------- + text + The folder's condition and biological repeat (``single`` for none), ``=``, and the + repeat group; the group is the text after the last ``=``. + + Returns + ------- + tuple + ``((condition, biological_repeat), group)``. + + Raises + ------ + DesignEditError + If the text is not in that form. + """ + folder, equals, group = text.rpartition("=") + condition, slash, repeat = folder.partition("/") + if not equals or not slash or not condition or not repeat or "/" in repeat: + raise DesignEditError( + f"{text!r} is not CONDITION/REPEAT=GROUP (for example ctrl/rep1=day1; the " + "biological repeat is single when the condition has no repeat folders)" + ) + return (condition, repeat), group + + +def apply_design_edit(design: ExperimentDesign, edit: DesignEdit, on: str) -> ExperimentDesign: + """Return the design with the changes made, refusing any that cannot be made. + + Parameters + ---------- + design + The design as read from ``experiment_design.csv``. + edit + The changes. + on + The date of the confirmations, ``YYYY-MM-DD``. + + Returns + ------- + ExperimentDesign + The changed design; a repeat group confirmed before keeps its date. + + Raises + ------ + DesignEditError + For an unknown folder, condition or repeat group, a condition both marked and + unmarked, a repeat group to confirm whose rows break the design rules + (``repeat_group_conflict``, ``invalid_experiment_design``), or a design whose values + cannot be written. The message says which and why. + """ + design = _moved(design, edit.repeat_groups) + design = _controls(design, edit.control, edit.not_control) + groups = design.unconfirmed_groups if edit.confirm_all else edit.confirm + _known("repeat group", groups, design.groups) + for group in dict.fromkeys(groups): + if not design.group_confirmed(group): + _check_group(design, group) + design = design.with_confirmed(group, True, on) + invalid = [i for i in design_issues(design) if i.code == "invalid_experiment_design"] + if invalid: + raise DesignEditError(invalid[0].message) + return design + + +def _moved(design: ExperimentDesign, moves: Mapping[tuple[str, str], str]) -> ExperimentDesign: + """Move folders into other repeat groups, refusing folders the design does not have.""" + folders = {row.key for row in design.rows} + unknown = [f"{c}/{r}" for c, r in moves if (c, r) not in folders] + if unknown: + known = ", ".join(f"{c}/{r}" for c, r in sorted(folders)) + raise DesignEditError( + f"unknown folder {', '.join(unknown)}: the design's folders are {known}" + ) + for key, group in moves.items(): + design = design.with_group(key, group) + return design + + +def _controls( + design: ExperimentDesign, control: tuple[str, ...], not_control: tuple[str, ...] +) -> ExperimentDesign: + """Mark and unmark controls, refusing unknown conditions and contradictions.""" + _known("condition", (*control, *not_control), design.conditions) + both = sorted(set(control) & set(not_control)) + if both: + raise DesignEditError(f"{', '.join(both)} cannot be marked a control and not one") + for condition in control: + design = design.with_control(condition, True) + for condition in not_control: + design = design.with_control(condition, False) + return design + + +def _known(what: str, given: Iterable[str], known: tuple[str, ...]) -> None: + """Refuse names the design does not have, listing those it has.""" + unknown = [name for name in dict.fromkeys(given) if name not in known] + if unknown: + raise DesignEditError( + f"unknown {what} {', '.join(map(repr, unknown))}: the design's {what}s are " + + (", ".join(known) or "none") + ) + + +def _check_group(design: ExperimentDesign, group: str) -> None: + """Refuse to confirm a repeat group whose rows break the design rules.""" + members = ExperimentDesign(design.members(group)) + problems = [issue for issue in design_issues(members) if issue.code != _UNCONFIRMED] + if problems: + raise DesignEditError( + f"repeat group {group!r} cannot be confirmed: " + + " ".join(issue.message for issue in problems) + ) diff --git a/src/morphobricks_preprocessing/report.py b/src/morphobricks_preprocessing/report.py index 6c6867b2..98dbb976 100644 --- a/src/morphobricks_preprocessing/report.py +++ b/src/morphobricks_preprocessing/report.py @@ -135,6 +135,54 @@ def design_summary(design: ExperimentDesign) -> str: return "\n".join(lines) +def design_table(design: ExperimentDesign) -> str: + """Show the design's rows as a table, then the repeat groups still to confirm. + + Parameters + ---------- + design + The experiment design. + + Returns + ------- + str + One row per biological-repeat folder of each condition (``experiment_design.csv`` + without its schema column), then which repeat groups are not confirmed yet. + """ + rows = [ + [ + "condition", + "biological repeat", + "repeat group", + "control", + "images", + "technical repeats", + "confirmed", + ] + ] + for row in design.rows: + on = f", {row.confirmed_on}" if row.confirmed_on else "" + confirmed = f"yes{on}" if row.confirmed else "no" + rows.append( + [ + row.condition, + row.biological_repeat, + row.repeat_group, + "yes" if row.control else "no", + str(row.image_count), + str(row.technical_repeat_count), + confirmed, + ] + ) + waiting = design.unconfirmed_groups + last = ( + f"Not confirmed yet: {', '.join(waiting)}" + if waiting + else "Every repeat group is confirmed." + ) + return format_table(rows) + "\n" + last + + def mapping_summary(mapping: AxisMapping, files: Iterable = ()) -> str: """Describe each axis on one line: its label, its length across files and its role.""" ranges = axis_length_ranges(mapping, files) diff --git a/src/morphobricks_preprocessing/run/__init__.py b/src/morphobricks_preprocessing/run/__init__.py index d9f27245..ab091235 100644 --- a/src/morphobricks_preprocessing/run/__init__.py +++ b/src/morphobricks_preprocessing/run/__init__.py @@ -23,7 +23,8 @@ from __future__ import annotations from .calibrate import write_calibration -from .check_step import write_check_report +from .check_step import CheckOutputs, write_check_outputs, write_check_report +from .design_file import edit_design, write_design_template from .lock import LOCK_FILENAME, LockHolder, OutputLockedError, read_lock, remove_stale_lock from .modes import FULL, MODES, RESTART from .pipeline import run_experiment @@ -35,16 +36,20 @@ "LOCK_FILENAME", "MODES", "RESTART", + "CheckOutputs", "LockHolder", "OutputLockedError", "Preflight", "RunResult", "SamplePlane", + "edit_design", "preflight", "read_lock", "remove_stale_lock", "run_experiment", "sample_planes", "write_calibration", + "write_check_outputs", "write_check_report", + "write_design_template", ] diff --git a/src/morphobricks_preprocessing/run/check_step.py b/src/morphobricks_preprocessing/run/check_step.py index d396e53c..348b4e7a 100644 --- a/src/morphobricks_preprocessing/run/check_step.py +++ b/src/morphobricks_preprocessing/run/check_step.py @@ -1,19 +1,23 @@ -"""``check``: the run's step stopped after the report; it writes only the report. +"""``check``: the run's step stopped after the report. ``check`` discovers the experiment and checks the headers like a run (design v0.4 §1), then writes ``discovery_report.json`` and its text version to the output folder, holding -the folder's lock while it writes. Nothing else is written. +the folder's lock while it writes. The command also writes the experiment design template, +prefilled from the folders, when the output folder holds no design +(:func:`write_check_outputs`, plan §12 question 57); it never replaces one. """ from __future__ import annotations import os +from dataclasses import dataclass from pathlib import Path from ..axes import AxisReview from ..discovery import check_output_folder, default_output_folder from ..discovery_report import RunOutcome, build_report -from ..validation import ValidationIssue +from ..validation import ValidationIssue, has_errors +from .design_file import write_design_template from .lock import OutputLockedError, lock_issue, output_lock from .preflight import Preflight from .publish import write_report @@ -42,15 +46,81 @@ def write_check_report( the experiment (nothing is written there; the preflight reports it); ``(None, issue)`` when another run holds the folder's lock. """ + written = _write(pre, output_folder, review, with_design=False) + return written.report, written.refusal + + +@dataclass(frozen=True) +class CheckOutputs: + """What ``check`` wrote to the output folder. + + Attributes + ---------- + report + ``discovery_report.json``, or None when nothing was written. + design + ``experiment_design.csv`` when the template was written; None when the folder holds + a design already, the folders have errors, or nothing was written. + refusal + ``output_locked`` when another run holds the folder's lock, else None. + """ + + report: Path | None = None + design: Path | None = None + refusal: ValidationIssue | None = None + + +def write_check_outputs( + pre: Preflight, + output_folder: str | os.PathLike | None = None, + review: AxisReview | None = None, +) -> CheckOutputs: + """Write the report of ``check``, and the design template when there is no design. + + Both are written while holding the output folder's lock. The template is prefilled + from the folders (:func:`.design_file.write_design_template`): nothing confirmed, no + control. It is written only when the folders have no error (as ``run`` asks for the + design only then), and never over a design already in the folder. + + Parameters + ---------- + pre + The preflight. + output_folder + The output folder; defaults to :func:`.discovery.default_output_folder`. + review + The axis review of the suggested roles, or None. + + Returns + ------- + CheckOutputs + Nothing written when the output folder is inside the experiment (the preflight + reports it) or another run holds its lock. + """ + return _write(pre, output_folder, review, with_design=True) + + +def _write( + pre: Preflight, + output_folder: str | os.PathLike | None, + review: AxisReview | None, + *, + with_design: bool, +) -> CheckOutputs: + """Write the report, and the design template ``with_design``, under the lock.""" root = pre.discovery.root output = default_output_folder(root) if output_folder is None else Path(output_folder) output = output.resolve() if check_output_folder(root, output): - return None, None + return CheckOutputs() output.mkdir(parents=True, exist_ok=True) report = build_report(pre, output, pre.issues, review=review, outcome=RunOutcome("check")) + folders_ok = not has_errors([*pre.discovery.issues, *pre.exclusion_issues]) try: with output_lock(output): - return write_report(report, output), None + design = None + if with_design and folders_ok: + design = write_design_template(output, pre.dataset.repeats) + return CheckOutputs(write_report(report, output), design) except OutputLockedError as exc: - return None, lock_issue(exc) + return CheckOutputs(refusal=lock_issue(exc)) diff --git a/src/morphobricks_preprocessing/run/design_file.py b/src/morphobricks_preprocessing/run/design_file.py new file mode 100644 index 00000000..00343c35 --- /dev/null +++ b/src/morphobricks_preprocessing/run/design_file.py @@ -0,0 +1,99 @@ +"""The output folder's ``experiment_design.csv``: its template, and changes made under the lock. + +``check`` writes the template, prefilled from the folders (:func:`write_design_template`), +when the output folder holds no design; it never replaces one. The ``design`` command +changes the design with :func:`edit_design`, which reads, changes and writes the file while +holding the output folder's lock, with the reader and validator the run uses +(:func:`.design.read_experiment_design`, :func:`.design.design_issues`). +""" + +from __future__ import annotations + +import os +from datetime import date +from pathlib import Path + +from ..dataset import BiologicalRepeat +from ..design import ( + DESIGN_FILENAME, + ExperimentDesign, + default_design, + read_experiment_design, + write_experiment_design, +) +from ..design_edit import DesignEdit, DesignEditError, apply_design_edit +from .lock import output_lock + + +def write_design_template(output: Path, repeats: tuple[BiologicalRepeat, ...]) -> Path | None: + """Write the design prefilled from the folders, unless the output folder holds one. + + The caller holds the output folder's lock. Each condition's biological-repeat folders + are rows; folders of the same name form one repeat group; nothing is confirmed and no + condition is a control (:func:`.design.default_design`). + + Parameters + ---------- + output + The output folder. + repeats + The biological-repeat folders found (:attr:`.dataset.DatasetReport.repeats`). + + Returns + ------- + Path or None + The file written; None when a design is already there (it is never replaced, also + when it cannot be read) or the experiment has no folders. + """ + path = output / DESIGN_FILENAME + if not repeats or path.exists() or path.is_symlink(): + return None + return write_experiment_design(default_design(repeats), path) + + +def edit_design( + output_folder: str | os.PathLike, edit: DesignEdit, *, on: str | None = None +) -> tuple[ExperimentDesign, Path | None]: + """Change ``experiment_design.csv`` in the output folder, holding the folder's lock. + + The file is read with the run's reader, changed (:func:`.design_edit.apply_design_edit`) + and written back only when something changed; a refused change leaves it as it is. + + Parameters + ---------- + output_folder + The output folder holding ``experiment_design.csv`` (written by ``check`` or ``run``). + edit + The changes; an empty one only reads and checks the design. + on + The date of the confirmations; today by default. + + Returns + ------- + tuple + The design after the change, and the file written, or None when nothing changed. + + Raises + ------ + DesignEditError + If the output folder holds no design, or a change is refused. + DesignFormatError + If the file cannot be read (the message names the line and column). + OutputLockedError + If a run holds the output folder's lock; nothing is read or written. + """ + output = Path(output_folder).resolve() + path = output / DESIGN_FILENAME + if not output.is_dir(): + raise DesignEditError(f"the output folder {output} does not exist: run 'check' first") + with output_lock(output): + if not path.is_file(): + raise DesignEditError( + f"{output} holds no {DESIGN_FILENAME}: 'check' or 'run' writes it, prefilled " + "from the experiment's folders" + ) + current = read_experiment_design(path) + changed = apply_design_edit(current, edit, on or date.today().isoformat()) + if changed == current: + return changed, None + return changed, write_experiment_design(changed, path) diff --git a/tests/run_files.py b/tests/run_files.py index 6a319c1f..b92eb6da 100644 --- a/tests/run_files.py +++ b/tests/run_files.py @@ -14,6 +14,12 @@ def only_the_report(folder): return json.loads((folder / REPORT_FILENAME).read_text(encoding="utf-8"))["status"] == "stopped" +def check_outputs(folder): + """True when ``check`` wrote its report and the design template, and nothing else.""" + names = {p.name for p in folder.iterdir()} if folder.is_dir() else set() + return names == {*REPORT_FILES, "experiment_design.csv"} + + def without_report(files): """Leave the report files out of ``{name: content}``: every run rewrites them.""" return {name: content for name, content in files.items() if name not in REPORT_FILES} diff --git a/tests/test_cli.py b/tests/test_cli.py index 2450420c..f1532142 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -14,7 +14,7 @@ write_experiment_design, ) from morphobricks_preprocessing.cli import main, parse_size -from run_files import only_the_report +from run_files import check_outputs from tiff_tree import write_data, write_file @@ -57,9 +57,11 @@ def test_check_prints_summary_and_suggested_axes(experiment, capsys): assert "Images to analyse: 3 in 2 leaf folders" in out assert "ctrl/" in out and "rep1/ (2 images; technical repeat: single)" in out assert "Suggested option for 'run': --axes ZCYX (axes in the files: ZCYX)" in out - # 'check' is the run's step stopped after the report: it writes only the report. + # 'check' is the run's step stopped after the report: it writes the report, and the + # design template prefilled from the folders (PP-8c). output = experiment.parent / "exp_MorphoBricks_Preprocessing" - assert only_the_report(output) and f"Report written to {output}" in out + assert check_outputs(output) and f"Report written to {output}" in out + assert f"The experiment design was written to {output / 'experiment_design.csv'}" in out def test_check_exits_with_1_on_errors(experiment, capsys): diff --git a/tests/test_design_command.py b/tests/test_design_command.py new file mode 100644 index 00000000..0940cc86 --- /dev/null +++ b/tests/test_design_command.py @@ -0,0 +1,142 @@ +"""PP-8c item 6: the experiment design from the folders, earlier and from the command line. + +- ``check`` writes the prefilled ``experiment_design.csv`` when the output folder holds + none (only ``run`` did); it never replaces a design, also one the user edited. +- ``design --output OUT`` prints the design and what is still to confirm, and changes it + without editing the file (``--confirm``, ``--confirm-all``, ``--control``, + ``--no-control``, ``--repeat-group``), under the lock, with the run's reader and + validator. Nothing is confirmed by default; anything unknown is refused, nothing changed. +- ``edit_design`` / ``DesignEdit`` do the same in Python. +""" + +import numpy as np +import pytest + +from morphobricks_preprocessing import read_experiment_design +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.run.lock import output_lock +from tiff_tree import write_data + +DESIGN = "experiment_design.csv" + + +@pytest.fixture +def experiment(tmp_path): + root = tmp_path / "exp" + rels = ["ctrl/day1/a.tif", "ctrl/day2/a.tif", "drug/day1/a.tif", "drug/d2/a.tif"] + for seed, rel in enumerate(rels): + data = np.random.default_rng(seed).integers(0, 200, (6, 8), dtype=np.uint8) + write_data(root / rel, data, "YX") + return root + + +@pytest.fixture +def checked(experiment, tmp_path): + output = tmp_path / "out" + assert main(["check", str(experiment), "--output", str(output)]) == 0 + return output + + +def design(output): + return read_experiment_design(output / DESIGN) + + +def test_check_writes_the_prefilled_template(checked): + found = design(checked) + assert [(r.condition, r.biological_repeat, r.repeat_group) for r in found.rows] == [ + ("ctrl", "day1", "day1"), + ("ctrl", "day2", "day2"), + ("drug", "d2", "d2"), + ("drug", "day1", "day1"), + ] + assert not any(r.confirmed or r.control for r in found.rows) + assert [r.image_count for r in found.rows] == [1, 1, 1, 1] + + +def test_check_never_replaces_a_design(experiment, checked): + path = checked / DESIGN + edited = path.read_text(encoding="utf-8").replace("ctrl,day1,day1,false", "ctrl,day1,day1,true") + edited = edited.replace("ctrl,day2,day2,false", "ctrl,day2,day2,true") + path.write_text(edited, encoding="utf-8") + assert main(["check", str(experiment), "--output", str(checked)]) == 0 + assert path.read_text(encoding="utf-8") == edited + + +def test_design_prints_the_table_and_what_is_unconfirmed(checked, capsys): + before = (checked / DESIGN).read_bytes() + assert main(["design", "--output", str(checked)]) == 0 + out = capsys.readouterr().out + assert "condition biological repeat repeat group control" in out + assert "Not confirmed yet: d2, day1, day2" in out + assert (checked / DESIGN).read_bytes() == before # nothing confirmed by default + + +def test_design_pairs_marks_and_confirms(checked, capsys): + args = ["design", "--output", str(checked), "--repeat-group", "drug/d2=day2"] + assert main([*args, "--control", "ctrl", "--confirm", "day2"]) == 0 + found = design(checked) + assert found.repeat_groups()[("drug", "d2")] == "day2" + assert found.controls == ("ctrl",) + assert found.group_confirmed("day2") and not found.group_confirmed("day1") + assert all(len(r.confirmed_on) == 10 for r in found.members("day2")) + assert "Not confirmed yet: day1" in capsys.readouterr().out + assert main(["design", "--output", str(checked), "--confirm-all"]) == 0 + assert design(checked).confirmed + assert main(["design", "--output", str(checked), "--no-control", "ctrl"]) == 0 + assert design(checked).controls == () + + +@pytest.mark.parametrize( + ("args", "message"), + [ + (["--confirm", "day9"], "unknown repeat group 'day9'"), + (["--control", "placebo"], "unknown condition 'placebo'"), + (["--repeat-group", "ctrl/day9=day1"], "unknown folder ctrl/day9"), + (["--repeat-group", "ctrl-day1"], "is not CONDITION/REPEAT=GROUP"), + (["--control", "ctrl", "--no-control", "ctrl"], "cannot be marked a control and not"), + (["--repeat-group", "drug/d2=NA"], "repeat_group"), + ], +) +def test_design_refuses_unknown_names_and_changes_nothing(checked, capsys, args, message): + before = (checked / DESIGN).read_bytes() + assert main(["design", "--output", str(checked), *args]) == 1 + out = capsys.readouterr().out + assert "The experiment design was not changed" in out and message in out + assert (checked / DESIGN).read_bytes() == before + + +def test_design_refuses_to_confirm_a_group_with_two_folders_of_a_condition(checked, capsys): + args = ["design", "--output", str(checked), "--repeat-group", "ctrl/day2=day1"] + assert main([*args, "--confirm", "day1"]) == 1 + out = capsys.readouterr().out + assert "repeat group 'day1' cannot be confirmed" in out and "only one folder" in out + assert not design(checked).group_confirmed("day1") + + +def test_design_without_a_design_file_says_how_to_get_one(tmp_path, capsys): + (tmp_path / "out").mkdir() + assert main(["design", "--output", str(tmp_path / "out")]) == 1 + assert "'check' or 'run' writes it" in capsys.readouterr().out + + +def test_design_is_refused_while_a_run_holds_the_lock(checked, capsys): + with output_lock(checked): + assert main(["design", "--output", str(checked), "--confirm-all"]) == 1 + assert "Another run is using this output folder" in capsys.readouterr().out + assert not design(checked).confirmed + + +def test_a_design_confirmed_with_the_command_lets_the_run_go(experiment, checked): + assert main(["design", "--output", str(checked), "--confirm-all"]) == 0 + assert main(["run", str(experiment), "--output", str(checked), "--axes", "YX"]) == 0 + + +def test_the_python_interface(checked): + from morphobricks_preprocessing import DesignEdit, DesignEditError, edit_design + + changed, path = edit_design(checked, DesignEdit(confirm=("day1",)), on="2026-11-02") + assert path == (checked / DESIGN).resolve() + assert changed.confirmed_on("day1") == "2026-11-02" + assert edit_design(checked, DesignEdit(confirm=("day1",)))[1] is None # nothing changed + with pytest.raises(DesignEditError, match="unknown repeat group"): + edit_design(checked, DesignEdit(confirm=("nope",))) diff --git a/tests/test_modes_lock_report.py b/tests/test_modes_lock_report.py index 5974618c..478df533 100644 --- a/tests/test_modes_lock_report.py +++ b/tests/test_modes_lock_report.py @@ -38,7 +38,7 @@ ) from morphobricks_preprocessing.run.lock import output_lock from morphobricks_preprocessing.testing import make_clean_run -from run_files import only_the_report +from run_files import check_outputs, only_the_report from tiff_tree import write_data, write_plain ZYX = prefill_mapping("ZYX") @@ -418,10 +418,10 @@ def test_a_moved_experiment_is_reported(experiment, tmp_path): assert {c.name for c in again.input_changes} == {"experiment_root"} -def test_check_writes_only_the_report(experiment): +def test_check_writes_the_report_and_the_design_template(experiment): assert main(["check", str(experiment)]) == 0 output = default_output_folder(experiment) - assert only_the_report(output) + assert check_outputs(output) data = json.loads((output / REPORT_FILENAME).read_text(encoding="utf-8")) assert data["step"] == "check" and data["mode"] is None assert data["images"]["ctrl/r1/a.tif"]["roles"] == "ZYX" From fce9337f09e89eda939aeab21a6e5047ca7da67d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 08:28:02 +0000 Subject: [PATCH 8/9] PP-8c: design_not_confirmed names the design command The action now offers the design command (--confirm) next to step 1 of the notebook and editing experiment_design.csv. design.py keeps its line count. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/user_guide.md | 2 +- src/morphobricks_preprocessing/design.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/user_guide.md b/docs/user_guide.md index b4bb1ab8..45e82f1b 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -699,7 +699,7 @@ continues. | `calibration_unreadable` | Error | `calibration.csv` cannot be read; the message names the line and what is wrong. The file is left as it is: correct it, or remove it to use the sizes in the metadata. | | `unbalanced_images` | Warning | A condition and biological repeat has fewer than half or more than twice the median number of images. Check for missing, misplaced or duplicated images. | | `repeat_missing_conditions` | Warning | A biological repeat is not found in every condition. It is still used. If it has another folder name in some condition, pair it in the experiment design (step 1). | -| `design_not_confirmed` | Error | Some repeat groups are not confirmed as comparable. Confirm them in step 1, or set `confirmed` to `true` in `experiment_design.csv`. | +| `design_not_confirmed` | Error | Some repeat groups are not confirmed as comparable. Confirm them in step 1, with the `design` command (`--confirm GROUP`, `--confirm-all`), or set `confirmed` to `true` in `experiment_design.csv`. | | `rgb_not_possible` | Error | The image type RGB was chosen, but the images are not three channels of a whole-number pixel type. Choose fluorescence, or check the axis roles: the RGB samples must have the role C. | | `repeat_group_conflict` | Error | A repeat group holds two folders of the same condition. Move one into another group. | | `invalid_experiment_design` | Error | A repeat group has no usable name (empty, `NA`, `nan`, `none`, `null`, `n/a`, ``, or spaces around it), two repeat groups are spelled the same apart from upper and lower case, or a confirmation date is not written `YYYY-MM-DD`. Correct `experiment_design.csv` (the message names the line and column). | diff --git a/src/morphobricks_preprocessing/design.py b/src/morphobricks_preprocessing/design.py index 2142ac97..01ae7c44 100644 --- a/src/morphobricks_preprocessing/design.py +++ b/src/morphobricks_preprocessing/design.py @@ -487,8 +487,8 @@ def design_issues(design: ExperimentDesign) -> list[ValidationIssue]: "that the biological repeats were prepared and acquired under the same " "circumstances, so that they are comparable.", [_folder(row) for g in unconfirmed for row in design.members(g)], - "Confirm the biological repeats in step 1 of the notebook, or set confirmed " - "to true for every row of these groups in experiment_design.csv.", + "Confirm the biological repeats in step 1 of the notebook, with the design " + "command (--confirm), or by setting confirmed to true in experiment_design.csv.", ) ) return issues From abcf59d4ea07b47831d4bcf34226e208e65e4931 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 09:10:50 +0000 Subject: [PATCH 9/9] PP-8c review: design and calibrate take --remove-stale-lock The output_locked message tells users to give --remove-stale-lock to the command, which design and calibrate did not take. Both now do, with the same rule as check, run and masks: removed only on that request, never the lock of a process still running on the same computer. Plan Q57, contract and user guide updated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 2 +- docs/plan.md | 4 + docs/user_guide.md | 7 +- src/morphobricks_preprocessing/cli.py | 19 +++-- .../cli_calibrate.py | 5 +- src/morphobricks_preprocessing/cli_design.py | 10 ++- tests/test_stale_lock_design_calibrate.py | 73 +++++++++++++++++++ 7 files changed, 105 insertions(+), 15 deletions(-) create mode 100644 tests/test_stale_lock_design_calibrate.py diff --git a/docs/contract.md b/docs/contract.md index a4b2430e..c537bc5d 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -253,7 +253,7 @@ the bricks' tests. | Experiment root | `run_experiment(experiment_root=None, output_folder=...)` and CLI `run --output FOLDER` use the recorded experiment root; without one, `ValueError` (API) or `experiment_not_recorded` (CLI). Another root than the recorded one is a change: every row is read again, and the recorded plain-stack confirmations are not applied there (each plain stack must be confirmed again). | | `masks` command | Uses the recorded inputs the same way (plan §12 question 56): the experiment argument is optional when `--output` holds a v2 manifest recording it (`experiment_not_recorded` otherwise), and the masks folder, mask-axis statements and exclusions are the recorded ones unless given (`--no-exclude` clears the exclusions). A value given that differs is used for that scan and printed as a change; the manifest is not changed (the next run records only what it is given). | | Axis roles | `mapping=None` uses the recorded roles; without any, the run stops with `axes_not_confirmed`. CLI `--axes` is needed for the first run and for plain stacks that are new or changed. | -| Lock | `.morphobricks-preprocessing.lock` in the output folder, created with `O_CREAT \| O_EXCL`, JSON `host`, `pid`, `started_at`; held by `run`, by `check` while it writes its report and by `masks` while it writes `mask_relationships.json` (CLI `run` also holds it to write the design template after a run stopped on the design), removed on every exit, and removed when the lock file cannot be written. A second run is refused with `output_locked`, naming the holder, and writes nothing. `remove_stale_lock(output)` / `--remove-stale-lock` removes a lock only on that request, never one of a process still running on the same computer (checked on POSIX). Not written inside the experiment. The lock is not part of the manifest. | +| Lock | `.morphobricks-preprocessing.lock` in the output folder, created with `O_CREAT \| O_EXCL`, JSON `host`, `pid`, `started_at`; held by `run`, by `check` while it writes its report (and the design template), by `masks` while it writes `mask_relationships.json`, and by `design` and `calibrate` while they change `experiment_design.csv` and `calibration.csv` (CLI `run` also holds it to write the design template after a run stopped on the design), removed on every exit, and removed when the lock file cannot be written. A second run is refused with `output_locked`, naming the holder, and writes nothing. `remove_stale_lock(output)` / `--remove-stale-lock` (every command) removes a lock only on that request, never one of a process still running on the same computer (checked on POSIX). Not written inside the experiment. The lock is not part of the manifest. | | Report | `discovery_report.json` (report version 1) and `discovery_report.txt`, written on every run that holds the lock, also when it stops: `step` (`run` or `check`), `status` (`stopped` or `written`; `check` is always `stopped`), `provisional` (true for the report written after the header checks, before any pixel is read), `manifest_sha256` (the manifest published with it, `written` only), `experiment_root` and `experiment_root_changed`, `output_folder`, `masks_folder`, `excluded`, `groups` (labels, position identifiers, image counts), `channels`, `channel_names`, `images` (per image `axes`, `roles`, `axes_from`, `pixel_sizes` by role with unit and source, `header`, `stated` or `pixels`, or `"uncalibrated"` without an X and a Y entry), `calibration` (`file`, `sha256` and the `statements`), `design`, `image_type`, `masks` (per type paired masks and images without one; unpaired and ambiguous masks), `mode`, `reused`, `recomputed` (file: reason), `removed`, `replaced` (file: what happened), `input_changes`, `issues` (code, severity, message, paths, action). For people only: bricks never read it. A published report is replaced atomically with the table and manifest, last. | | `check` | The same step stopped after the report; it writes the report, and the design template when the output folder holds no design (§12; owner decision of 2 November 2026, plan §12 question 57, which amends design v0.4 §1 "it writes only the report"). | diff --git a/docs/plan.md b/docs/plan.md index 78dc2c0d..4cd24185 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1231,3 +1231,7 @@ Raised during review; to be decided before the milestones that need them: a group that gained a folder since is asked again. Python: `edit_design` with `DesignEdit` (`apply_design_edit` without files). The confirmation stays an explicit act of the user: nothing confirms by default. Contract §12.* + *Decided in review (2 November 2026, owner): `design` and `calibrate` also take + `--remove-stale-lock`, as `check`, `run` and `masks` do (the `output_locked` message + tells users to give it to the command). Same rule: the lock is removed only on that + request, and never one of a process still running on the same computer.* diff --git a/docs/user_guide.md b/docs/user_guide.md index 45e82f1b..c8858ca0 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -442,7 +442,7 @@ report names the change and its effect. Other options: | `--mode restart\|full` | `restart` (default) reuses the rows of unchanged images; `full` recomputes every row and check. Both keep your confirmations and replace the results only when the run succeeds | | `--no-masks` | Drop the masks folder an earlier run recorded | | `--no-exclude` | Clear the exclusions an earlier run recorded, so every image is analysed again (not together with `--exclude`) | -| `--remove-stale-lock` | Remove the lock left by a run that was stopped or whose computer restarted (`check` and `run`); give it only when no other run uses the output folder | +| `--remove-stale-lock` | Remove the lock left by a run that was stopped or whose computer restarted (`check`, `run`, `masks`, `design` and `calibrate`); give it only when no other run uses the output folder | | `--outlier-limit N`, `--saturation-limit FRACTION`, `--min-images N` | Limits of the intensity checks printed after the run (default 3, 0.01 and 5) | | `--log FILE` | Write technical details to a file | | `--masks FOLDER` | The masks folder (§2). `check` lists the mask types found and checks every mask from its header; `run` stops on a mask error and records the masks in `run_manifest.json` | @@ -611,8 +611,9 @@ with `chmod a+r /*`, or delete them before running again. Only one run at a time can use an output folder: a run holds a lock file (`.morphobricks-preprocessing.lock`) and a second run is refused (`output_locked`). If the process is forcibly stopped, its lock stays; remove it with `--remove-stale-lock` once you -are sure no run is going on. `check` and `masks` hold the lock while they write their -reports, and are refused the same way. A stopped run leaves the last good results in place; +are sure no run is going on (every command takes it). `check` and `masks` hold the lock +while they write their reports, `design` and `calibrate` while they change their files, +and are refused the same way. A stopped run leaves the last good results in place; rerunning checks the table against the manifest and rebuilds a mismatched pair. `mask_relationships.json` is removed by a `full` run and when the masks changed since it was written; run the `masks` command again. diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index 8673ee00..4ef29bdb 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -88,8 +88,8 @@ def build_parser() -> argparse.ArgumentParser: ) _common(masks, recorded=True) add_masks_arguments(masks) - add_calibrate_parser(commands) - add_design_parser(commands) + for command in (add_calibrate_parser(commands), add_design_parser(commands)): + _remove_stale_lock_option(command) return parser @@ -98,10 +98,10 @@ def main(argv: Sequence[str] | None = None) -> int: _safe_output() parser = build_parser() args = parser.parse_args(argv) - if args.command == "calibrate": - return calibrate_command(args) - if args.command == "design": - return design_command(args) + if args.command in ("calibrate", "design"): + if args.remove_stale_lock and not _remove_stale_lock(args): + return 1 + return {"calibrate": calibrate_command, "design": design_command}[args.command](args) if getattr(args, "no_exclude", False) and args.exclude: parser.error("--exclude and --no-exclude cannot be given together") handler = _attach_log(args.log) if getattr(args, "log", None) else None @@ -153,6 +153,11 @@ def _common(parser: argparse.ArgumentParser, *, recorded: bool = False) -> None: "folders, outside the experiment; the label images of OME-Zarr images are added", ) _mask_axes_option(parser) + _remove_stale_lock_option(parser) + + +def _remove_stale_lock_option(parser: argparse.ArgumentParser) -> None: + """Add ``--remove-stale-lock``, which every command holding the folder's lock takes.""" parser.add_argument( "--remove-stale-lock", action="store_true", @@ -178,7 +183,7 @@ def _remove_stale_lock(args: argparse.Namespace) -> bool: """Remove the output folder's lock the user said is stale; False when it is not.""" if args.output: output = Path(args.output).resolve() - elif args.experiment: + elif getattr(args, "experiment", None): # 'design' takes only --output output = default_output_folder(Path(args.experiment).resolve()) else: print("Give the experiment folder or --output to say whose lock to remove.") diff --git a/src/morphobricks_preprocessing/cli_calibrate.py b/src/morphobricks_preprocessing/cli_calibrate.py index d9a2ed5f..cc654073 100644 --- a/src/morphobricks_preprocessing/cli_calibrate.py +++ b/src/morphobricks_preprocessing/cli_calibrate.py @@ -30,8 +30,8 @@ from .run.lock import lock_issue -def add_calibrate_parser(commands: argparse._SubParsersAction) -> None: - """Add the ``calibrate`` command and its options.""" +def add_calibrate_parser(commands: argparse._SubParsersAction) -> argparse.ArgumentParser: + """Add the ``calibrate`` command and its options; return its parser.""" parser = commands.add_parser( "calibrate", help="show or state the pixel sizes in calibration.csv in the output folder", @@ -69,6 +69,7 @@ def add_calibrate_parser(commands: argparse._SubParsersAction) -> None: help="remove the statements for TARGET along AXES; the image metadata are used " "again (repeatable)", ) + return parser def calibrate_command(args: argparse.Namespace) -> int: diff --git a/src/morphobricks_preprocessing/cli_design.py b/src/morphobricks_preprocessing/cli_design.py index 80cc7cb3..22277ded 100644 --- a/src/morphobricks_preprocessing/cli_design.py +++ b/src/morphobricks_preprocessing/cli_design.py @@ -22,13 +22,18 @@ from .run.lock import lock_issue -def add_design_parser(commands: argparse._SubParsersAction) -> None: - """Add the ``design`` command and its options. +def add_design_parser(commands: argparse._SubParsersAction) -> argparse.ArgumentParser: + """Add the ``design`` command and its options; return its parser. Parameters ---------- commands The command line's subcommands. + + Returns + ------- + argparse.ArgumentParser + The ``design`` command's parser. """ design = commands.add_parser( "design", @@ -73,6 +78,7 @@ def add_design_parser(commands: argparse._SubParsersAction) -> None: action="store_true", help="confirm every repeat group not confirmed yet", ) + return design def design_command(args: argparse.Namespace) -> int: diff --git a/tests/test_stale_lock_design_calibrate.py b/tests/test_stale_lock_design_calibrate.py new file mode 100644 index 00000000..1f385e39 --- /dev/null +++ b/tests/test_stale_lock_design_calibrate.py @@ -0,0 +1,73 @@ +"""PP-8c review: ``design`` and ``calibrate`` take ``--remove-stale-lock`` like the others. + +The lock message (``output_locked``) tells users to give ``--remove-stale-lock`` to the +command. ``check``, ``run`` and ``masks`` took it; ``design`` and ``calibrate``, which also +hold the output folder's lock, did not. The option removes a lock only on that request, and +never one of a process still running on the same computer. +""" + +import os + +import numpy as np +import pytest + +from morphobricks_preprocessing import read_experiment_design +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.run.lock import read_lock +from test_modes_lock_report import finished_pid, stale_lock +from tiff_tree import write_data + + +@pytest.fixture +def checked(tmp_path): + root, output = tmp_path / "exp", tmp_path / "out" + for seed, rel in enumerate(["ctrl/day1/a.tif", "drug/day1/a.tif"]): + data = np.random.default_rng(seed).integers(0, 200, (6, 8), dtype=np.uint8) + write_data(root / rel, data, "YX") + assert main(["check", str(root), "--output", str(output)]) == 0 + return output + + +DESIGN = ["design", "--confirm-all"] +CALIBRATE = ["calibrate", "--size", ".", "XY", "0.5", "um"] + + +def confirmed(output): + return read_experiment_design(output / "experiment_design.csv").confirmed + + +def stated(output): + return (output / "calibration.csv").exists() + + +@pytest.mark.parametrize(("args", "done"), [(DESIGN, confirmed), (CALIBRATE, stated)]) +def test_a_stale_lock_is_removed_only_on_request(checked, capsys, args, done): + stale_lock(checked, finished_pid(), host="another-computer") + command = [*args, "--output", str(checked)] + assert main(command) == 1 + out = capsys.readouterr().out + assert "Another run is using this output folder" in out and "--remove-stale-lock" in out + assert not done(checked) and read_lock(checked) is not None + assert main([*command, "--remove-stale-lock"]) == 0 + out = capsys.readouterr().out + assert "Removed the lock of process" in out and "another-computer" in out + assert done(checked) and read_lock(checked) is None + + +@pytest.mark.skipif(os.name != "posix", reason="only POSIX can tell a process runs") +@pytest.mark.parametrize(("args", "done"), [(DESIGN, confirmed), (CALIBRATE, stated)]) +def test_the_lock_of_a_running_process_on_this_computer_is_kept(checked, capsys, args, done): + stale_lock(checked, os.getpid()) + assert main([*args, "--output", str(checked), "--remove-stale-lock"]) == 1 + assert "The lock was kept" in capsys.readouterr().out + assert not done(checked) and read_lock(checked).pid == os.getpid() + + +def test_calibrate_finds_the_lock_from_the_experiment_folder(tmp_path, capsys): + root = tmp_path / "exp" + write_data(root / "ctrl" / "a.tif", np.zeros((6, 8), dtype=np.uint8), "YX") + output = tmp_path / "exp_MorphoBricks_Preprocessing" + output.mkdir() + stale_lock(output, finished_pid(), host="another-computer") + assert main([*CALIBRATE, str(root), "--remove-stale-lock"]) == 0 + assert stated(output) and read_lock(output) is None