From 41f45f6272d55708a263d4f938ca72398f465500 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 07:53:04 +0000 Subject: [PATCH 1/2] Fill position_id per image and record it in the run manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-brick decision D02 (audit finding X02): a field of view is identified in every brick by condition + biological_repeat + technical_repeat + position_id, with position_id falling back to the image file stem only when one file is exactly one position. Here it always is, so: - discovery.position_id: the image name without .tif/.tiff/.zarr and a preceding .ome; exported from the package. - New error duplicate_position_id: two images in one leaf folder with the same identifier, ignoring case (img_01.tif and img_01.ome.zarr), are refused instead of guessed. - Every plane_histograms.csv row carries position_id; rows reused from an earlier table with it blank are filled without reading pixels. CSV columns and schema version 2 unchanged. - run_manifest.json: optional per-file key position_id (null when absent); manifest version stays 1, like earlier optional keys. - Contract §1/§5/§8/§9, plan §6/§10/§12 question 20, user guide and AGENTS.md updated; tests for equal names in two technical repeats, duplicate refusal, manifest round trip, OME-Zarr and converted images, and refilling an earlier table. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U6vJfhGWQ7pTpsJhw48Zf7 --- AGENTS.md | 17 ++++- docs/contract.md | 11 ++- docs/plan.md | 24 +++++++ docs/user_guide.md | 13 +++- src/morphobricks_preprocessing/__init__.py | 2 + src/morphobricks_preprocessing/discovery.py | 62 +++++++++++++++++ src/morphobricks_preprocessing/manifest.py | 11 ++- src/morphobricks_preprocessing/run.py | 18 ++++- tests/test_discovery.py | 49 +++++++++++++ tests/test_manifest.py | 12 ++++ tests/test_run.py | 76 +++++++++++++++++++++ tests/test_zarr_input.py | 8 +++ 12 files changed, 291 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 00061314..affaa77b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,7 +57,8 @@ Pipeline: ``` src/morphobricks_preprocessing/ - discovery.py folder walk, leaf folders, labels (condition / biological_repeat / technical_repeat) + discovery.py folder walk, leaf folders, labels (condition / biological_repeat / technical_repeat), + position_id per image and the duplicate check metadata.py TIFF header inspection (tifffile, no pixel reads), dtype/dimension/series checks, OME channel names + significant bits validation.py ValidationIssue (severity, code, message, paths, action), error()/warning() builders @@ -154,6 +155,11 @@ functions the tests use. path. - **Depth:** a folder below the technical-repeat level is an **error** (so a layer of images is never dropped silently). +- **Positions (cross-brick decision D02):** one image is one position. Its `position_id` + (`discovery.position_id`) is the name without `.tif`/`.tiff`/`.zarr` and a preceding + `.ome`. The position key is `condition` + `biological_repeat` + `technical_repeat` + + `position_id`. Two images in one leaf folder with the same identifier, ignoring case, + are the error `duplicate_position_id`: never guess. - **System files:** dot-files and dot-folders, `Thumbs.db`, `desktop.ini` and `__MACOSX` are ignored **without** a warning. - **Warnings:** non-TIFF files, empty folders, TIFFs outside leaf folders. @@ -229,6 +235,8 @@ functions the tests use. - **`plane_id`** = relative path plus `::z=…,c=…,t=…`, always all three (`img.tif::z=0,c=0,t=0` for a 2D image). It is the row key. - **Rows** are sorted by path, then z, c, t. +- **`position_id`** is filled in every row (D02, plan §12 question 20). Earlier tables with + it blank are filled on the next run without reading pixels. - **Reading:** labels are always read back as text (`NA`, `001`, `TRUE` stay as typed). Floats are written with `repr`. @@ -236,7 +244,8 @@ functions the tests use. - **Manifest contents:** - settings, the confirmed labels per leaf, and the axis mapping; - - per-file size, `mtime_ns`, axes, `singleton_roles` and plane count; + - per-file size, `mtime_ns`, axes, `singleton_roles`, plane count and `position_id` + (optional key, null when absent; manifest version stays 1); - channel names and significant bits (OME only, information only); - the CSV's SHA-256 and package versions (including `morphobricks-normalization`). - **A file is unchanged** when its size, mtime, labels and axis layout match and the table @@ -315,6 +324,7 @@ Technical details go to `logging` (shown in the notebook's "Technical details" p | CLI | `check ` (summary, label tree, issues, suggested `--axes`) and `run --axes ROLES` with `--output`, `--exclude`, `--bins`, `--memory-limit 2GiB`, `--missing-as-singleton`, `--force-rebuild`, `--log`, and the conversion options above. Axis roles are always stated explicitly, never taken silently from metadata. Exit 1 on errors | | Large planes (Q15) | Planes over the memory limit are read in blocks automatically, announced by the warning `planes_read_in_blocks` and `read_in_blocks` in the manifest. Block size derived from the limit (whole strips/tiles/chunks, uncompressed pieces and chunks also by rows). Compressed single-strip/tile planes keep the `plane_over_memory_limit` error. Two-pass histograms identical to whole planes. OME-Zarr copies written block by block, smaller levels built from the stored level above. Axis preview reduced (every n-th pixel) with a caption saying so | | Normalization brick (P1) | The modules `dtypes`, `histogram`, `records`, `csv_io`, `pooling` and `normalization` moved unchanged to `morphobricks_normalization` (normalization plan N1, N2, N6) and are imported from there. Their public names stay importable from `morphobricks_preprocessing` and from the old submodules for one version: module `__getattr__` stand-ins (`_moved.py`), one `DeprecationWarning` per old path naming `morphobricks_normalization..`, same objects. The package's own code never uses the old paths. Outputs unchanged (CSV schema 2); the manifest's `versions` adds `morphobricks-normalization`; Step 0 checks for it; contract §3–§7 text moved to the normalization contract (§1.2, §8.2) | +| Position identity (Q20, D02) | `position_id` = image name without extension, in every CSV row and manifest file entry; FOV key = labels + `position_id`; duplicates in one leaf folder are the error `duplicate_position_id`. CSV columns and schema 2 unchanged; manifest version 1 with an optional key | | Notebook style | Follow the lab's NucleiSky and LabChronicle notebooks (see §7) | | Widget widths | **99%**, never 100% | | Paths and filenames in widget HTML and notebook text | **`…` / `**…**`, never `` or inline code** | @@ -584,7 +594,8 @@ jupyter lab notebooks/MorphoBricks_Preprocessing.ipynb written (PR #14, question 17), OME-NGFF label readers (PR #15, question 18), and follow-up P1 of the normalization plan: the dependency on `morphobricks-normalization` v0.1.0 with deprecated re-exports, its version in the manifest, Step 0 checking for it, - and contract §3–§7 moved to the normalization contract (PRs #16 and #17, question 19). + and contract §3–§7 moved to the normalization contract (PRs #16 and #17, question 19), + and `position_id` filled per cross-brick decision D02 (question 20). - **Next (not started):** follow-ups P2–P5 of the normalization plan (modality, references, colour statistics, previews). - **Not yet verified:** diff --git a/docs/contract.md b/docs/contract.md index e2313555..ceabb82a 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -28,6 +28,7 @@ whole experiment; any change to them goes through the normalization brick. | Depth | At most three folder levels below the root. A folder below the technical-repeat level is an error. | | System files | Dot-files and dot-folders (`.DS_Store`, `._*`, `.ipynb_checkpoints`), `Thumbs.db`, `desktop.ini` and `__MACOSX` are ignored without a warning. | | Scope | A group is identified by its full label path, e.g. `(control, bio_rep1)`. | +| Positions | Every image is exactly one position (field of view): files with several positions, scenes, series or tiles are refused (§2). Its `position_id` is the image name without its extension (`.tif`, `.tiff` or `.zarr`, any case, with a preceding `.ome`): `img_01.ome.tif` gives `img_01`. A position is identified by `condition`, `biological_repeat`, `technical_repeat` and `position_id` (cross-brick decision D02, 26 October 2026), so equal names in two technical repeats are two positions. Two images in one leaf folder with the same `position_id`, ignoring case (`img_01.tif` and `img_01.ome.zarr`), are an error (`duplicate_position_id`): the brick does not guess which is which. | | Paths | `source_relative_path` is relative to the experiment root, with `/` separators. | ## 2. Axes @@ -71,6 +72,12 @@ Defined by the [normalization brick's contract][normalization contract] §8.2 (c §8.1 (shared format). This brick writes it next to `run_manifest.json` (§9), with plane coordinates and labels as in §1–§2. +This brick fills `position_id` in every row, with the image's position identifier (§1). +The file-name fallback follows cross-brick decision D02 (one file is exactly one position +here); the normalization contract's note that `position_id` is never inferred from file +names predates D02. The columns and `schema_version` are unchanged. A table written before +this rule, with `position_id` blank, is filled on the next run without reading pixels. + ## 6. Group statistics Defined by the [normalization brick's contract][normalization contract] §1.2.3: groups @@ -85,7 +92,7 @@ method is an offset and a scale, `(pixels − offset) / scale`. | Severity | Situations | Effect | |---|---|---| -| Error | Mixed dtypes; unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images, or converting TIFF images to OME-Zarr, without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles; every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single`; sibling folders differing only in case; folder below the technical-repeat level; output folder inside the experiment; no usable planes | Processing is blocked | +| Error | Mixed dtypes; unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images, or converting TIFF images to OME-Zarr, without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles; every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single`; sibling folders differing only in case; images in one leaf folder with the same position identifier (§1); folder below the technical-repeat level; output folder inside the experiment; no usable planes | Processing is blocked | | Warning | Files that are not images; empty folders; images outside leaf folders; images excluded by the user, or excluded paths that match no image; channel names differing between files; non-finite pixels; different image sizes with a valid mapping; planes without finite pixels while others remain; planes over the memory limit that are read in blocks | Processing continues; the item is reported | Every message states what happened in plain language, lists the affected paths, and @@ -123,7 +130,7 @@ file's schema or statistical interpretation. | `settings` | `bin_count`, `memory_limit` (bytes), `missing_as_singleton`, `exclude` (sorted relative paths), `convert_tiffs` (whether TIFF images are converted to OME-Zarr and processed from the copies; false when absent), `conversion` (`chunk_size`, `compression`, `compression_level`, `pyramid_levels` — null for automatic; defaults when absent). | | `axis_mapping` | `axes` (file axis labels) and `roles` (`X`, `Y`, `Z`, `C`, `T` or null per axis). | | `labels` | One entry per leaf folder: `folder` (relative path, `.` for the root) and its three labels. | -| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count`, `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | +| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count`, `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent), `position_id` (the image's position identifier, §1; null when absent, in manifests written before it). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | | `csv_sha256` | SHA-256 of the table written with this manifest. | | `versions` | Versions of `morphobricks-preprocessing`, `morphobricks-normalization` (which computes the histograms and writes the CSV; absent in manifests written before it), `numpy`, `tifffile` and Python. Information only: a different version never causes a rebuild. | diff --git a/docs/plan.md b/docs/plan.md index c9b595d3..3725af9a 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -168,6 +168,7 @@ Columns, in order: `position_id, image_file, z, c, t, condition, biological_repe technical_repeat, source_relative_path, plane_id, dtype, bit_depth, pixel_min, pixel_max, total_pixel_count, valid_pixel_count, binning_min, binning_max, bin_count, schema_version`, then `histogram_bin_00000` onwards. Details in contract §5. +`position_id` is the image name without its extension (question 20). The wide CSV is for computational use; spreadsheet programs may not open all columns. The package provides `read_plane_histograms()`, which keeps labels as text. Alongside the CSV, @@ -304,6 +305,10 @@ API, CLI). The notebook and CLI only call the API. and normalization modules come from `morphobricks-normalization` v0.1.0, a required dependency; the old names stay importable here with a `DeprecationWarning` for one version. Outputs are unchanged. *Implemented, see question 19.* +- **Position identity (cross-brick decision D02):** every row and every manifest file + entry carries the image's `position_id`, so `condition`, `biological_repeat`, + `technical_repeat` and `position_id` identify a position in every brick; images in one + leaf folder with the same identifier are refused. *Implemented, see question 20.* ## 11. Tests and acceptance criteria @@ -495,3 +500,22 @@ Raised during review; to be decided before the milestones that need them: the notebook's Step 0 installs the new dependency into environments set up before it; and the text of contract §3–§7 moved to the normalization brick's contract (§1.2 and §8.2), leaving short pointers here.* +20. The CSV has a `position_id` column that this brick left blank, and the run manifest + had no position per file. How are positions identified across bricks (cross-brick + audit finding X02)? + *Decided (October 2026, cross-brick decision D02 of 26 October 2026): a position + (field of view) is identified in every brick by `condition`, `biological_repeat`, + `technical_repeat` and `position_id`; `position_id` falls back to the image file name + without its extension only when one file is exactly one position. Here it always is + (several series and position-like axes are errors: question 2 and contract §2), so + every row's `position_id` is the image name without `.tif`, `.tiff` or `.zarr` (and a + preceding `.ome`): `img_01.ome.tif` gives `img_01`. Equal names in two technical + repeats are two positions. Two images in one leaf folder with the same identifier, + ignoring case (`img_01.tif` and `img_01.ome.zarr`), are the error + `duplicate_position_id` rather than a guess; excluding one of them does not resolve + it, renaming or moving one does. The manifest gains the optional per-file key + `position_id` (null when absent) and stays at version 1, like the optional keys of + questions 12 and 15. The CSV columns and schema version 2 are unchanged; an earlier + table with blank `position_id` is filled on the next run from the paths, without + reading pixels. The normalization contract's note "never inferred from file names" + (§8.2) predates D02 and is to be aligned in that repository.* diff --git a/docs/user_guide.md b/docs/user_guide.md index 51d65333..11822318 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -76,6 +76,11 @@ experiment/ are reported and skipped. - Do not name a folder `single`, and do not use two neighbouring folder names that differ only in upper/lower case (`Control` and `control`). Both are errors. +- Each image is one position (field of view), named after its file without the extension: + `img_01.tif` and `img_01.ome.zarr` are both position `img_01`. The same name may be used + in different folders (`tech_rep1/img_01.tif` and `tech_rep2/img_01.tif` are two + positions), but two images in one folder must not share a name apart from the extension + or upper/lower case. That is an error; rename one or move it out of the experiment. - A fourth level of folders (below the technical repeat) is an error, so that no layer of images is dropped silently. - Other files (`.txt`, `.csv`, ...) are reported and skipped. System files such as @@ -252,15 +257,16 @@ Both commands exit with status 1 when an error blocks processing. The default output folder is next to the experiment folder: `/data/exp1` → `/data/exp1_MorphoBricks_Preprocessing`. -- **`plane_histograms.csv`:** one row per plane with the file name, the zero-based plane +- **`plane_histograms.csv`:** one row per plane with the position (`position_id`, the file + name without its extension), the file name, the zero-based plane coordinates `z`, `c`, `t` (0 when the image has no such dimension), the three labels, the pixel type, the exact minimum and maximum, pixel counts and the histogram. The table is very wide (one column per bin), so spreadsheet programs may not open it completely; read it in Python with `read_plane_histograms()`. The columns are defined in the [normalization brick's contract](https://github.com/CellMigrationLab/morphobricks-normalization/blob/main/docs/contract.md) §8.2. - **`run_manifest.json`:** what the run read and with which settings: labels, axis roles, - bin count, memory limit, excluded images, package versions, and each image's size and - modification time. Channel names and significant bits are recorded when the metadata + bin count, memory limit, excluded images, package versions, and each image's position, + size and modification time. Channel names and significant bits are recorded when the metadata provides them, and whether the image's planes were too large to read whole and are read in blocks. Format in [contract.md](contract.md) §9. - **`ome_zarr/`** (only when TIFF images are converted): one `.ome.zarr` folder per TIFF @@ -308,6 +314,7 @@ continues. | `unreadable_folder` | Error | A folder could not be opened. Check permissions. | | `reserved_folder_name` | Error | A folder is named `single`. Rename it. | | `case_conflict` | Error | Neighbouring folders differ only in case. Rename or merge them. | +| `duplicate_position_id` | Error | Two images in one folder have the same name apart from the extension or upper/lower case (`img_01.tif` and `img_01.ome.zarr`), so their positions cannot be told apart. Rename one, or move it out of the experiment. | | `folder_too_deep` | Error | Folders below the technical repeat. Move the images up, or select a lower folder as the experiment. | | `output_inside_experiment` | Error | The output folder is inside the experiment. Choose another. | | `no_images` | Error | No images to analyse (or all excluded). Check the folder. | diff --git a/src/morphobricks_preprocessing/__init__.py b/src/morphobricks_preprocessing/__init__.py index afb03604..aa766962 100644 --- a/src/morphobricks_preprocessing/__init__.py +++ b/src/morphobricks_preprocessing/__init__.py @@ -28,6 +28,7 @@ check_output_folder, default_output_folder, discover_experiment, + position_id, ) from .manifest import ( MANIFEST_FILENAME, @@ -179,6 +180,7 @@ def __dir__() -> list[str]: "percentile_range", "plane_layout", "pooled_statistics", + "position_id", "prefill_experiment_mapping", "prefill_mapping", "preflight", diff --git a/src/morphobricks_preprocessing/discovery.py b/src/morphobricks_preprocessing/discovery.py index 1f67f081..fcc7e375 100644 --- a/src/morphobricks_preprocessing/discovery.py +++ b/src/morphobricks_preprocessing/discovery.py @@ -9,6 +9,10 @@ folders whose names end in ``.zarr`` (usually ``.ome.zarr``). An OME-Zarr folder counts as one image, not as a subfolder, and is never entered. +Each image is one position (field of view): files with several positions are +refused when their headers are checked. A position is identified by its labels +and its ``position_id``, the image name without its extension (contract §1). + Only folder and file names are read here; image headers are inspected by :mod:`morphobricks_preprocessing.metadata`. """ @@ -38,6 +42,10 @@ ROOT = "." +# Image name endings removed to give the position identifier, longest first, so +# that "img_01.ome.tif" and "img_01.ome.zarr" both give "img_01". +IMAGE_SUFFIXES = (".ome.tiff", ".ome.tif", ".ome.zarr", ".tiff", ".tif", ".zarr") + def is_system_name(name: str) -> bool: """Return True for system files and folders that are skipped silently. @@ -61,6 +69,34 @@ def is_ome_zarr_folder(entry: os.DirEntry) -> bool: return entry.is_dir() and is_zarr_name(entry.name) +def position_id(image: str) -> str: + """Return the position identifier of an image: its name without the extension. + + Every image is exactly one position, because files holding several + positions, scenes or tiles are refused, so the file name identifies the + position within its technical repeat. The full position (field of view) key + is ``condition``, ``biological_repeat``, ``technical_repeat`` and this + identifier (contract §1). The extension is ``.tif``, ``.tiff`` or ``.zarr`` + (any case), together with a preceding ``.ome``. + + Parameters + ---------- + image + The image's path relative to the experiment root, or its name. + + Returns + ------- + str + For example ``"img_01"`` for ``"ctrl/rep1/img_01.ome.tif"``. + """ + name = image.replace("\\", "/").rsplit("/", 1)[-1] + lowered = name.lower() + for suffix in IMAGE_SUFFIXES: + if lowered.endswith(suffix) and len(name) > len(suffix): + return name[: -len(suffix)] + return name + + def default_output_folder(experiment_root: str | os.PathLike) -> Path: """Return the default output folder, a sibling of the experiment folder. @@ -217,6 +253,7 @@ def __init__(self, root: Path): self.non_tiff: list[str] = [] self.empty: list[str] = [] self.outside_leaf: list[str] = [] + self.same_position: list[str] = [] def relative(self, path: Path) -> str: return path.relative_to(self.root).as_posix() or ROOT @@ -249,6 +286,7 @@ def visit(self, folder: Path, names: tuple[str, ...]) -> None: if tiffs: labels = names + (SINGLE,) * (len(LEVELS) - len(names)) self.leaves.append(LeafFolder(self.relative(folder), *labels, tuple(tiffs))) + self._check_positions(tiffs) else: self.empty.append(self.relative(folder)) return @@ -275,6 +313,17 @@ def _check_names(self, subfolders: list[Path]) -> None: if len(paths) > 1: self.case_conflicts.extend(self.relative(path) for path in paths) + def _check_positions(self, images: list[str]) -> None: + # Two images of one technical repeat with the same position identifier would + # be the same field of view to every later brick, so this is refused rather + # than guessed. Case is ignored for the same reason as for folder names. + by_key: dict[str, list[str]] = {} + for image in images: + by_key.setdefault(position_id(image).casefold(), []).append(image) + for paths in by_key.values(): + if len(paths) > 1: + self.same_position.extend(paths) + def finish(self) -> tuple[ValidationIssue, ...]: issues = self.issues if self.unreadable: @@ -319,6 +368,19 @@ def finish(self) -> tuple[ValidationIssue, ...]: "folder lower in the hierarchy as the experiment folder.", ) ) + if self.same_position: + issues.append( + error( + "duplicate_position_id", + "Some images in the same folder have the same name apart from the file " + "extension or upper/lower case (for example img_01.tif and img_01.ome.zarr). " + "Each image is one position, named after its file, so these positions " + "could not be told apart.", + sorted(self.same_position), + "Rename the images so that their names differ by more than the extension " + "or case, or move the extra copies out of the experiment folder.", + ) + ) if self.non_tiff: issues.append( warning( diff --git a/src/morphobricks_preprocessing/manifest.py b/src/morphobricks_preprocessing/manifest.py index 472ccf89..22ef68ac 100644 --- a/src/morphobricks_preprocessing/manifest.py +++ b/src/morphobricks_preprocessing/manifest.py @@ -28,6 +28,7 @@ from morphobricks_normalization.csv_io import SCHEMA_VERSION from .axes import AxisMapping +from .discovery import position_id from .zarr_export import ConversionSettings MANIFEST_FILENAME = "run_manifest.json" @@ -104,6 +105,10 @@ class FileEntry: read_in_blocks True when the image's planes are too large for the memory limit and are read in blocks. For information only: the rows are the same either way. + position_id + The image's position identifier, its name without the extension + (:func:`~morphobricks_preprocessing.discovery.position_id`); None in + manifests written before it was recorded. """ relative_path: str @@ -117,6 +122,7 @@ class FileEntry: significant_bits: int | None = None ome_zarr_path: str | None = None read_in_blocks: bool = False + position_id: str | None = None def same_source(self, other: FileEntry) -> bool: """Return True if ``other`` describes the same unchanged file, read the same way.""" @@ -267,7 +273,7 @@ def file_entry_for( ome_zarr_path: str | None = None, read_in_blocks: bool = False, ) -> FileEntry: - """Describe a source image as it is on disk now.""" + """Describe a source image as it is on disk now, with its position identifier.""" size, mtime_ns = source_fingerprint(Path(root) / relative_path) return FileEntry( relative_path=relative_path, @@ -281,6 +287,7 @@ def file_entry_for( significant_bits=significant_bits, ome_zarr_path=ome_zarr_path, read_in_blocks=read_in_blocks, + position_id=position_id(relative_path), ) @@ -376,6 +383,7 @@ def _file_dict(entry: FileEntry) -> dict: "significant_bits": entry.significant_bits, "ome_zarr_path": entry.ome_zarr_path, "read_in_blocks": entry.read_in_blocks, + "position_id": entry.position_id, } @@ -397,6 +405,7 @@ def _file_entry(data: dict) -> FileEntry: significant_bits=None if bits is None else int(bits), ome_zarr_path=None if data.get("ome_zarr_path") is None else str(data["ome_zarr_path"]), read_in_blocks=bool(data.get("read_in_blocks", False)), + position_id=None if data.get("position_id") is None else str(data["position_id"]), ) diff --git a/src/morphobricks_preprocessing/run.py b/src/morphobricks_preprocessing/run.py index f7655e47..3b2708e1 100644 --- a/src/morphobricks_preprocessing/run.py +++ b/src/morphobricks_preprocessing/run.py @@ -405,20 +405,30 @@ def stop(**counts) -> RunResult: records: list[PlaneRecord] = [] for rel in unchanged if not reason else (): - records.extend(previous_records[rel]) + # Tables written before position identifiers were recorded have them blank; + # the identifier depends only on the path, so it is filled without reading. + records.extend( + replace(record, position_id=entries[rel].position_id) + for record in previous_records[rel] + ) failed: list[str] = [] try: for done, rel in enumerate(to_read): if progress is not None: progress(done, len(to_read), rel) file_mapping = review.file_mappings[rel] + position = entries[rel].position_id try: if rel in to_convert: # Convert first, then read the copy like any other OME-Zarr image. copy, copy_mapping = staging.convert(rel, file_mapping, headers[rel], settings) - records.extend(_read_file(copy, rel, labels[rel], copy_mapping, settings)) + records.extend( + _read_file(copy, rel, labels[rel], copy_mapping, settings, position) + ) else: - records.extend(_read_file(root / rel, rel, labels[rel], file_mapping, settings)) + records.extend( + _read_file(root / rel, rel, labels[rel], file_mapping, settings, position) + ) except PlaneReadError: logger.warning("Cannot read the planes of %s", rel, exc_info=True) failed.append(rel) @@ -600,6 +610,7 @@ def _read_file( labels: tuple[str, str, str], mapping: AxisMapping, settings: RunSettings, + position: str | None, ) -> list[PlaneRecord]: """Histogram every plane of ``path``, recording the rows under the source ``rel``.""" records = [] @@ -614,6 +625,7 @@ def _read_file( z=plane.z, c=plane.c, t=plane.t, + position_id=position, ) ) return records diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 1f92bca2..c99d8735 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -8,6 +8,7 @@ check_output_folder, default_output_folder, discover_experiment, + position_id, ) from morphobricks_preprocessing.discovery import is_system_name, is_tiff_name from tiff_tree import make_tree @@ -268,6 +269,54 @@ def test_folders_below_technical_repeat_are_an_error(tmp_path): assert result.images == ("cond/bio/tech2/a.tif",) +# Positions (cross-brick decision D02) ------------------------------------------------- + + +@pytest.mark.parametrize( + ("image", "expected"), + [ + ("a.tif", "a"), + ("ctrl/rep1/tech1/img_01.tiff", "img_01"), + ("ctrl/img_01.ome.tif", "img_01"), + ("IMG_01.OME.TIFF", "IMG_01"), + ("ctrl/img_01.ome.zarr", "img_01"), + ("ctrl/img_01.zarr", "img_01"), + ("ctrl\\001.tif", "001"), + ("a.b.tif", "a.b"), + ("ome.tif", "ome"), + ], +) +def test_position_id_is_the_image_name_without_extension(image, expected): + assert position_id(image) == expected + + +def test_equal_names_in_different_technical_repeats_are_fine(tmp_path): + make_tree(tmp_path, ["ctrl/bio1/tech1/img_01.tif", "ctrl/bio1/tech2/img_01.tif"]) + result = discover_experiment(tmp_path) + assert result.issues == () + keys = {(*leaf.labels, position_id(image)) for leaf in result.leaves for image in leaf.images} + assert keys == {("ctrl", "bio1", "tech1", "img_01"), ("ctrl", "bio1", "tech2", "img_01")} + + +@pytest.mark.parametrize( + "names", + [ + ("img_01.tif", "img_01.ome.zarr/"), + ("img_01.tif", "img_01.tiff"), + ("img_01.ome.tif", "img_01.tif"), + ("IMG_01.tiff", "img_01.tif"), + ], +) +def test_images_with_the_same_position_id_in_one_folder_are_an_error(tmp_path, names): + make_tree(tmp_path, [f"ctrl/tech1/{name}" for name in names] + ["ctrl/tech2/img_01.tif"]) + result = discover_experiment(tmp_path) + assert not result.ok + found = issue(result, "duplicate_position_id") + assert found.severity is Severity.ERROR + assert found.paths == tuple(sorted(f"ctrl/tech1/{name.rstrip('/')}" for name in names)) + assert found.action + + def test_output_folder_inside_experiment_is_an_error(tmp_path): make_tree(tmp_path, ["exp/cond/a.tif", "exp/results/plane_histograms.csv"]) result = discover_experiment(tmp_path / "exp", output_folder=tmp_path / "exp" / "results") diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 7133cf62..32f64fbd 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -132,11 +132,23 @@ def test_read_in_blocks_round_trips_and_defaults_to_false(tmp_path): assert not read_manifest(path).files[0].read_in_blocks +def test_position_id_round_trips_and_is_none_when_absent(tmp_path): + original = manifest(files=(entry(position_id="a"), entry("cond/rep/b.tif", position_id="b"))) + path = write_manifest(original, tmp_path / "m.json") + assert read_manifest(path) == original + data = json.loads(path.read_bytes()) + assert [f["position_id"] for f in data["files"]] == ["a", "b"] + del data["files"][0]["position_id"] # written before position identifiers were recorded + path.write_text(json.dumps(data), encoding="utf-8") + assert read_manifest(path).files[0].position_id is None + + def test_file_entry_for_reads_size_and_time(tmp_path): path = write_tiff(tmp_path / "a.tif") found = file_entry_for(tmp_path, "a.tif", ("single",) * 3, prefill_mapping("YX")) stat = os.stat(path) assert (found.size, found.mtime_ns, found.axes) == (stat.st_size, stat.st_mtime_ns, "YX") + assert found.position_id == "a" def test_file_sha256(tmp_path): diff --git a/tests/test_run.py b/tests/test_run.py index 30b50b95..a9a7d59b 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -1,10 +1,13 @@ +import hashlib import json import os +from dataclasses import replace from pathlib import Path import numpy as np import pytest import tifffile +from morphobricks_normalization.csv_io import write_plane_histograms from morphobricks_preprocessing import ( MANIFEST_FILENAME, @@ -190,6 +193,79 @@ def test_manifest_records_channel_names_and_significant_bits(tmp_path): assert manifest.file("img_0.ome.tif").significant_bits == 12 +# Positions (cross-brick decision D02) ------------------------------------------------- + + +def test_every_row_has_its_position_id(experiment): + result = run_experiment(experiment, ZYX) + records = read_plane_histograms(result.csv_path) + assert {r.position_id for r in records} == {"img_01", "img_02"} + # The same file name in two technical repeats gives two distinct positions. + keys = { + (r.condition, r.biological_repeat, r.technical_repeat, r.position_id) + for r in records + if r.image_file == "img_01.tif" + } + assert keys == { + ("control", "bio_rep1", "tech_rep1", "img_01"), + ("control", "bio_rep1", "tech_rep2", "img_01"), + ("treated", "bio_rep1", SINGLE, "img_01"), + ("treated", "bio_rep2", "tech_rep1", "img_01"), + } + # Position key plus plane coordinates identifies every row. + rows = [ + (r.condition, r.biological_repeat, r.technical_repeat, r.position_id, r.z, r.c, r.t) + for r in records + ] + assert len(set(rows)) == len(rows) == 2 * len(IMAGES) + manifest = read_manifest(result.manifest_path) + assert {f.relative_path: f.position_id for f in manifest.files} == { + rel: rel.rsplit("/", 1)[-1].removesuffix(".tif") for rel in IMAGES + } + data = json.loads(result.manifest_path.read_text(encoding="utf-8")) + assert data["manifest_version"] == "1" and data["schema_version"] == "2" + assert all("position_id" in f for f in data["files"]) + + +def test_position_ids_that_are_text_stay_text(tmp_path): + root = tmp_path / "exp" + for seed, rel in enumerate(["ctrl/001.tif", "ctrl/NA.tif", "drug/TRUE.tif"]): + write_data(root / rel, stack(seed), "ZYX") + result = run_experiment(root, ZYX) + records = read_plane_histograms(result.csv_path) + assert sorted({r.position_id for r in records}) == ["001", "NA", "TRUE"] + + +def test_images_with_the_same_position_id_are_refused(experiment): + write_data(experiment / "control/bio_rep1/tech_rep1/img_01.ome.tif", stack(9), "ZYX") + result = run_experiment(experiment, ZYX) + assert not result.written and not result.output_folder.exists() + assert issue(result.issues, "duplicate_position_id").paths == ( + "control/bio_rep1/tech_rep1/img_01.ome.tif", + "control/bio_rep1/tech_rep1/img_01.tif", + ) + assert not preflight(experiment).ok + + +def test_table_without_position_ids_is_filled_without_reading(experiment, reads): + first = run_experiment(experiment, ZYX) + table = csv_bytes(first) + # A table and manifest written before position identifiers were recorded. + blank = [replace(r, position_id=None) for r in read_plane_histograms(first.csv_path)] + write_plane_histograms(blank, first.csv_path) + data = json.loads(first.manifest_path.read_text(encoding="utf-8")) + for f in data["files"]: + del f["position_id"] + data["csv_sha256"] = hashlib.sha256(first.csv_path.read_bytes()).hexdigest() + first.manifest_path.write_text(json.dumps(data), encoding="utf-8") + reads.clear() + result = run_experiment(experiment, ZYX) + assert result.written and result.rebuild_reason is None + assert result.unchanged == tuple(sorted(IMAGES)) and reads == [] + assert csv_bytes(result) == table + assert all(f.position_id for f in read_manifest(result.manifest_path).files) + + def test_custom_output_folder(experiment, tmp_path): result = run_experiment(experiment, ZYX, output_folder=tmp_path / "out" / "nested") assert result.written and result.csv_path == tmp_path / "out" / "nested" / OUTPUT_FILENAME diff --git a/tests/test_zarr_input.py b/tests/test_zarr_input.py index 9fb25b0d..51606bf2 100644 --- a/tests/test_zarr_input.py +++ b/tests/test_zarr_input.py @@ -479,6 +479,11 @@ def test_ome_zarr_experiment_end_to_end(zarr_experiment): np.testing.assert_array_equal(record.histogram.counts, expected.counts) assert record.t == 0 assert records[0].plane_id == "ctrl/rep1/a.ome.zarr::z=0,c=0,t=0" + assert {(r.condition, r.position_id) for r in records} == { + ("ctrl", "a"), + ("ctrl", "b"), + ("drug", "a"), + } def test_rerun_detects_a_changed_store(zarr_experiment): @@ -541,6 +546,9 @@ def test_conversion_leaves_ome_zarr_images_alone(tmp_path): manifest = read_manifest(result.manifest_path) assert manifest.file("b.ome.zarr").ome_zarr_path is None assert manifest.file("a.tif").ome_zarr_path == "ome_zarr/a.ome.zarr" + # A converted image keeps the position identifier of its source. + assert {r.position_id for r in read_plane_histograms(result.csv_path)} == {"a", "b"} + assert manifest.file("a.tif").position_id == "a" def test_sample_planes_cover_both_formats(tmp_path): From 353be9d641a80a5fcdbc99d3b70e23f1e40df8ea Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 09:15:04 +0000 Subject: [PATCH 2/2] Require Python 3.11 (MorphoBricks decision D01) - requires-python >=3.11 and Ruff target py311; the two upgrades Ruff then asks for: datetime.UTC, and Severity as a StrEnum (it is only read through .value, so text output is unchanged). - Notebook Step 0 and its test now refuse Python 3.10. - CI matrix 3.11 and 3.13; the 3.11 jobs pin zarr 2.18 so both zarr majors stay tested, as the 3.10 jobs did before. - AGENTS, development guide and user guide say 3.11. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01U6vJfhGWQ7pTpsJhw48Zf7 --- .github/workflows/ci.yml | 5 ++++- AGENTS.md | 6 +++--- docs/development.md | 2 +- docs/user_guide.md | 2 +- notebooks/MorphoBricks_Preprocessing.ipynb | 4 ++-- pyproject.toml | 4 ++-- src/morphobricks_preprocessing/run.py | 4 ++-- src/morphobricks_preprocessing/validation.py | 4 ++-- tests/test_notebook_setup.py | 4 ++-- 9 files changed, 19 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c39d51e..b9d1c654 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,11 +23,14 @@ jobs: fail-fast: false matrix: os: [ubuntu-latest, windows-latest, macos-latest] - python-version: ["3.10", "3.13"] + python-version: ["3.11", "3.13"] steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} - run: python -m pip install -e ".[dev,notebook,zarr]" + # zarr 3 needs Python 3.11 too, so pin 2.18 on 3.11 to keep both majors tested. + - if: matrix.python-version == '3.11' + run: python -m pip install "zarr>=2.18,<3" - run: python -m pytest diff --git a/AGENTS.md b/AGENTS.md index affaa77b..d432e85c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,7 +128,7 @@ functions the tests use. *Summary* (by module), *Decisions*, *Notes for review* (limitations, what was not verified), *Tests* (exact counts, e.g. "455 passed, 2 skipped"). 5. **Done means:** `ruff check .`, `ruff format --check .` and `pytest` all pass (CI runs - lint on Ubuntu and tests on Ubuntu, Windows and macOS with Python 3.10 and 3.13), docs + lint on Ubuntu and tests on Ubuntu, Windows and macOS with Python 3.11 and 3.13), docs updated, and local changes ready for review. Open a PR only when the user requests it. 6. **Be honest in reports.** Say what was verified and what was not (for example: "widgets tested headless; not looked at in a real JupyterLab"). @@ -335,7 +335,7 @@ Technical details go to `logging` (shown in the notebook's "Technical details" p ## 6. Programming style -- **Python 3.10+.** Put `from __future__ import annotations` in every module, and type +- **Python 3.11+** (MorphoBricks decision D01). Put `from __future__ import annotations` in every module, and type hints on all public functions. - **Ruff is the authority.** Line length 100. Rules E, W, F, I, B, UP, NPY and D, with numpy docstring convention; D105 and D107 ignored; tests are exempt from D. @@ -397,7 +397,7 @@ Technical details go to `logging` (shown in the notebook's "Technical details" p - **Notebook widgets are tested headless:** build the pages, drive them with `button.click()` and `.value = …`, and assert on `.value` of HTML widgets. Tests skip if ipywidgets/matplotlib are missing; CI installs `.[dev,notebook,zarr]`. OME-Zarr - tests skip without zarr; CI's Python 3.10 jobs get zarr 2.18 and 3.13 jobs zarr 3. + tests skip without zarr; CI's Python 3.11 jobs pin zarr 2.18 and 3.13 jobs get zarr 3. - **The notebook file is tested too:** - valid nbformat 4, with no outputs; - code cells hidden and titled `# @markdown ### Run to …`; diff --git a/docs/development.md b/docs/development.md index 5930e8a8..af0a17df 100644 --- a/docs/development.md +++ b/docs/development.md @@ -2,7 +2,7 @@ ## Environment -- Python 3.10 or newer. +- Python 3.11 or newer. - Core dependencies: `numpy`, `tifffile` and `morphobricks-normalization` (installed from its GitHub release tag; it holds the histogram, CSV, pooling and normalization code). Notebook dependencies (`ipywidgets`, diff --git a/docs/user_guide.md b/docs/user_guide.md index 11822318..b277ba6a 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -12,7 +12,7 @@ its own output folder, which is never inside the experiment folder. ## 1. Installation -You need Python 3.10 or newer and JupyterLab (or Jupyter Notebook). Download +You need Python 3.11 or newer and JupyterLab (or Jupyter Notebook). Download [`notebooks/MorphoBricks_Preprocessing.ipynb`](../notebooks/MorphoBricks_Preprocessing.ipynb), open it in Jupyter and run **Step 0**. It detects local Jupyter or Google Colab and checks these options: diff --git a/notebooks/MorphoBricks_Preprocessing.ipynb b/notebooks/MorphoBricks_Preprocessing.ipynb index 9dfe4625..d7d96366 100644 --- a/notebooks/MorphoBricks_Preprocessing.ipynb +++ b/notebooks/MorphoBricks_Preprocessing.ipynb @@ -103,8 +103,8 @@ "def _start_preprocessing_notebook():\n", " \"\"\"Prepare this kernel's environment and return the notebook interface.\"\"\"\n", " # This standalone cell can run before the package's Python requirement is checked.\n", - " if sys.version_info < (3, 10): # noqa: UP036\n", - " raise RuntimeError(\"Select a Python 3.10 or newer notebook kernel, then run Step 0 again.\")\n", + " if sys.version_info < (3, 11): # noqa: UP036\n", + " raise RuntimeError(\"Select a Python 3.11 or newer notebook kernel, then run Step 0 again.\")\n", "\n", " environment = \"Google Colab\" if \"google.colab\" in sys.modules else \"local Jupyter\"\n", " print(f\"Checking the {environment} environment...\", flush=True)\n", diff --git a/pyproject.toml b/pyproject.toml index c1f2b45f..0d381d5d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,7 +9,7 @@ description = "MorphoBricks preprocessing brick: per-plane intensity histograms readme = "README.md" license = "MIT" license-files = ["LICENSE"] -requires-python = ">=3.10" +requires-python = ">=3.11" authors = [{ name = "CellMigrationLab" }] keywords = ["microscopy", "image analysis", "histogram", "normalization", "MorphoBricks"] classifiers = [ @@ -58,7 +58,7 @@ addopts = "-ra" [tool.ruff] line-length = 100 -target-version = "py310" +target-version = "py311" src = ["src", "tests"] [tool.ruff.lint] diff --git a/src/morphobricks_preprocessing/run.py b/src/morphobricks_preprocessing/run.py index 3b2708e1..6f3d6c7c 100644 --- a/src/morphobricks_preprocessing/run.py +++ b/src/morphobricks_preprocessing/run.py @@ -23,7 +23,7 @@ from collections.abc import Callable, Iterable, Iterator, Sequence from contextlib import contextmanager from dataclasses import dataclass, replace -from datetime import datetime, timezone +from datetime import UTC, datetime from pathlib import Path from morphobricks_normalization.csv_io import ( @@ -490,7 +490,7 @@ def stop(**counts) -> RunResult: ), csv_sha256=file_sha256(csv_path), versions=package_versions(), - written_at=datetime.now(timezone.utc).isoformat(timespec="seconds"), + written_at=datetime.now(UTC).isoformat(timespec="seconds"), ) write_manifest(manifest, staged / MANIFEST_FILENAME) logger.info( diff --git a/src/morphobricks_preprocessing/validation.py b/src/morphobricks_preprocessing/validation.py index 34efa894..b8f2e6a4 100644 --- a/src/morphobricks_preprocessing/validation.py +++ b/src/morphobricks_preprocessing/validation.py @@ -9,10 +9,10 @@ from collections.abc import Iterable from dataclasses import dataclass -from enum import Enum +from enum import StrEnum -class Severity(str, Enum): +class Severity(StrEnum): """How serious an issue is.""" ERROR = "error" diff --git a/tests/test_notebook_setup.py b/tests/test_notebook_setup.py index 39608bc0..6476eedd 100644 --- a/tests/test_notebook_setup.py +++ b/tests/test_notebook_setup.py @@ -172,8 +172,8 @@ def test_setup_reports_colab_without_importing_or_mounting_drive( def test_setup_rejects_old_python_before_installing(setup_environment, monkeypatch): - monkeypatch.setattr(sys, "version_info", (3, 9, 0)) - with pytest.raises(RuntimeError, match="Python 3.10 or newer"): + monkeypatch.setattr(sys, "version_info", (3, 10, 0)) + with pytest.raises(RuntimeError, match="Python 3.11 or newer"): setup_environment.start() assert setup_environment.installs == setup_environment.imports == []