From 523c9517486e7dcb5b65d78632df591235b6417e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 20:14:20 +0000 Subject: [PATCH 1/5] Micrometre conversions are positive and finite; check and design exit 1 on errors (#93) - length_units (new, pure): the unit table, space_unit and to_micrometers, moved from physical_sizes (re-exported there). to_micrometers refuses a length that overflows or underflows in micrometres (1e308 m, 1e-320 pm) and an int beyond the float range with a ValueError naming it. - PhysicalSizes.micrometers converts with to_micrometers: None for such a size, never inf; _same_length is False for a non-finite size, so inf is never "the same" as another size. - Header sizes (TIFF and OME-Zarr) that do not convert, and an OME-Zarr scale product that overflows, are None with the reason in PhysicalSizes.invalid, named by invalid_physical_calibration; calibration_entry refuses a stated one. - CLI: check exits 1 on a design it cannot read, design exits 1 when it prints repeat_group_conflict or invalid_experiment_design. - Contract, user guide, plan question 66, AGENTS map; baseline lowered (physical_sizes.py now under 600 lines). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- AGENTS.md | 4 +- docs/contract.md | 11 +- docs/plan.md | 29 +++ docs/user_guide.md | 14 +- pyproject.toml | 1 + src/morphobricks_preprocessing/calibration.py | 10 +- src/morphobricks_preprocessing/cli.py | 21 +- src/morphobricks_preprocessing/cli_design.py | 3 +- .../length_units.py | 161 ++++++++++++ .../physical_sizes.py | 196 ++++----------- .../pixel_size_checks.py | 7 +- tests/test_cli_exit_status.py | 65 +++++ tests/test_micrometre_conversion.py | 238 ++++++++++++++++++ tools/code_rules_baseline.json | 1 - 14 files changed, 583 insertions(+), 178 deletions(-) create mode 100644 src/morphobricks_preprocessing/length_units.py create mode 100644 tests/test_cli_exit_status.py create mode 100644 tests/test_micrometre_conversion.py diff --git a/AGENTS.md b/AGENTS.md index dde83760..4a79226a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -69,9 +69,11 @@ src/morphobricks_preprocessing/ header consistency warnings (width/height, plane counts, pixel sizes); header_axes and named_axes (the axes the metadata names, contract §16) physical_sizes.py PhysicalSizes (pixel sizes, z-step, time step, a unit per axis, micrometers() - for comparing), space_unit/to_micrometers (the shared unit rule, contract §16), + for comparing), re-exports length_units' space_unit/to_micrometers, read_physical_sizes (OME / ImageJ TIFF metadata; also importable ) and ngff_physical_sizes (OME-NGFF scale and axis units) + length_units.py space_unit/to_micrometers, SPACE_UNITS (the shared unit rule, contract + §16): a conversion is always a positive finite length, else ValueError (#93) ngff_scale.py read_scale: an OME-NGFF coordinateTransformations scale, checked; what is wrong with a malformed one (PhysicalSizes.invalid, issue #94) dataset.py dataset consistency from the folders: biological repeats per condition, balance diff --git a/docs/contract.md b/docs/contract.md index fe541740..575ba1f5 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -56,7 +56,7 @@ channel order; an image without a C axis has one channel. | OME-Zarr labels | The label images of an image's OME-NGFF `labels` group (0.4 and 0.5) are listed with `ngff_label_names` and opened, level 0, with `open_ngff_labels` (by name; the name may be left out when there is one). They are never analysed; later bricks such as patch extraction use them as masks. | | Experiment mapping | Defined on the TIFF arrangement when the experiment has TIFF files; in an experiment of OME-Zarr images only, on their arrangement with the most axes. | | Channel names, significant bits | Recorded in the run manifest when metadata provides them; not part of the CSV. Channel names that differ at the same channel index are the error `channel_names_differ` (issue #37): channels are matched by their number, so two stains at one number would be pooled as one channel. Only named channels are compared; a channel without a name in some images is not. Names are read without leading and trailing whitespace, for every image (TIFF, OME-Zarr and this brick's own copies), so `DAPI ` and `DAPI` are one name; the manifest records it (`channel_names_trimmed`, §9). Whitespace inside a name is compared exactly (`DA PI` and `DAPI` differ), with no override. An OME-Zarr channel label that is not text is malformed metadata: the image is unreadable (`unreadable_ome_zarr`), never read as an unnamed channel. | -| Physical sizes | Read with the header, not part of the CSV or the manifest, and compared between images in the folder check (§11, in micrometres with each axis's own unit; a size without a known unit cannot be compared): `PhysicalSizes` with the size of one step along X, Y, Z and T and each axis's unit (OME-NGFF unit names). TIFF: OME `PhysicalSize{X,Y,Z}` with their own units (micrometres when left out) and `TimeIncrement`, or ImageJ resolution, `spacing` and `finterval` with `unit`, `yunit`, `zunit`. OME-Zarr (0.4 and 0.5): the `scale` of level 0's `coordinateTransformations`, times the multiscales entry's own `scale` when given, with each axis's `unit` (common spellings such as `µm`, `um`, `micron` and `s` given as UDUNITS-2 names, others as written). Length units are the microscopy units of MorphoBricks decision D-a2 (§16): `pm`, `Å`/`angstrom`, `nm`, `µm` (`um`, `micron(s)`, `micrometer(s)`, `micrometre(s)`), `mm`, `cm`, `m`, and the names of each in US and British spelling (`nanometre`, ...); ImageJ's escaped micro sign (`\u00B5m`) is recognised. A size in any other unit (`inch`, `decimeter`, ...) is not converted to micrometres, so it cannot be compared: that axis is uncalibrated, and the unit as written is kept in `PhysicalSizes.unsupported_units` (axis letter and unit, information only, not part of equality) so that the folder check names it (`unsupported_pixel_size_unit`, §11). A plain TIFF's resolution tags (no OME or ImageJ metadata, for example 300 dpi with the unit inch) are never a pixel size (decision D-c): such an image has no calibration. Nothing is guessed: what the metadata does not give is None. Nothing is dropped silently either (issue #94): a calibration given but malformed (`coordinateTransformations` that are not a list, a transformation of a type other than `scale`, `translation` and `identity`, more than one scale, a scale in a separate file (`path`) or not one number per axis) gives no sizes, and a scale value of X, Y, Z or T that is not a positive finite number gives none on that axis; each is recorded with what is wrong (`PhysicalSizes.invalid`, `ngff_scale.read_scale`) and named by the warning `invalid_physical_calibration` (§11); an OME-Zarr scale without a unit is kept with the unit None, except on the axes a multiscales entry's `metadata` lists as `unknown_scale` (copies written by this brick up to v0.3, §10), whose sizes are None. | +| Physical sizes | Read with the header, not part of the CSV or the manifest, and compared between images in the folder check (§11, in micrometres with each axis's own unit; a size without a known unit cannot be compared): `PhysicalSizes` with the size of one step along X, Y, Z and T and each axis's unit (OME-NGFF unit names). TIFF: OME `PhysicalSize{X,Y,Z}` with their own units (micrometres when left out) and `TimeIncrement`, or ImageJ resolution, `spacing` and `finterval` with `unit`, `yunit`, `zunit`. OME-Zarr (0.4 and 0.5): the `scale` of level 0's `coordinateTransformations`, times the multiscales entry's own `scale` when given, with each axis's `unit` (common spellings such as `µm`, `um`, `micron` and `s` given as UDUNITS-2 names, others as written). Length units are the microscopy units of MorphoBricks decision D-a2 (§16): `pm`, `Å`/`angstrom`, `nm`, `µm` (`um`, `micron(s)`, `micrometer(s)`, `micrometre(s)`), `mm`, `cm`, `m`, and the names of each in US and British spelling (`nanometre`, ...); ImageJ's escaped micro sign (`\u00B5m`) is recognised. A size in any other unit (`inch`, `decimeter`, ...) is not converted to micrometres, so it cannot be compared: that axis is uncalibrated, and the unit as written is kept in `PhysicalSizes.unsupported_units` (axis letter and unit, information only, not part of equality) so that the folder check names it (`unsupported_pixel_size_unit`, §11). A plain TIFF's resolution tags (no OME or ImageJ metadata, for example 300 dpi with the unit inch) are never a pixel size (decision D-c): such an image has no calibration. Nothing is guessed: what the metadata does not give is None. Nothing is dropped silently either (issue #94): a calibration given but malformed (`coordinateTransformations` that are not a list, a transformation of a type other than `scale`, `translation` and `identity`, more than one scale, a scale in a separate file (`path`) or not one number per axis) gives no sizes, and a scale value of X, Y, Z or T that is not a positive finite number gives none on that axis; each is recorded with what is wrong (`PhysicalSizes.invalid`, `ngff_scale.read_scale`) and named by the warning `invalid_physical_calibration` (§11). The same holds for a size that is not a positive finite length in micrometres once converted (issue #93: `1e308 m` overflows to infinity, `1e-320 pm` underflows to zero; also an OME-Zarr level-0 scale times the image's scale that does so), from any TIFF or OME-Zarr metadata: the size is None, never `inf`, and `PhysicalSizes.invalid` gives the axis and the value; an OME-Zarr scale without a unit is kept with the unit None, except on the axes a multiscales entry's `metadata` lists as `unknown_scale` (copies written by this brick up to v0.3, §10), whose sizes are None. | | Plane counts | Images may have different numbers of depth slices, time points or planes along an unassigned dimension; this is a warning (§11). | | Channel counts | Every image of an experiment has the same number of channels (normalization plan N43): along the C axis, 1 without one (also when a missing C axis is confirmed as length 1). A different number is the error `different_channel_counts`, found before any image is read: in the folder check from the headers (the `C` dimension, or the RGB samples `S` when there is no `C`), and again with the confirmed axis roles, which decide (§11). | @@ -267,6 +267,7 @@ the bricks' tests. | 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` from before its first mask read to the write of `mask_relationships.json` (a locked folder is refused before any mask is read), 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`, `conversion` (for a tree of named-axis copies, §17: `record`, `sha256`, `source_root`; null otherwise), `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"). The notebook's **Check folder** does the same with the same functions (`run.check_step.suggested_review`, `write_check_outputs`; v0.4.0, PP-10): the report, and the template with the design card; nothing when an embedding brick hides the output folder (`folder_step(show_output=False)`). | +| Exit status (v0.4.0) | Every command (`check`, `run`, `masks`, `calibrate`, `design`, `convert`) exits with status 1 when it prints an error and 0 otherwise; warnings never change it, and options argparse cannot read exit with 2. `check` exits with 1 on every error it reports: the preflight's, the axis check's, a refused write of its outputs (`output_locked`) and an experiment design it cannot read (`unreadable_experiment_design`; found in the review of #130, it exited with 0). `design` exits with 1 when the design it prints has `repeat_group_conflict` or `invalid_experiment_design`, also after a change it wrote (it exited with 0); `design_not_confirmed` is not printed by `design` and does not count. | | Notebook (v0.4.0, PP-10) | Step 3 offers the mode (`restart`, `full`) with its effect, and lists before the run the inputs that differ from the recorded ones with their effect (`run.planned.planned_changes(given, output_folder, design=, calibration=)`, from `resolve_inputs`, `design_for_run` and `calibration_change`: the changes the run then reports). After a run it names the mode, the files replaced and the input changes, and shows the report; a stopped run says what it wrote (question 55). A refusal `output_locked` (check, run, a design answer or a stated size) shows the holder (computer, process ID, start time) and a **Remove the lock of a stopped run** button: `remove_stale_lock` only on that click, never the lock of a process still running on the same computer; on Google Drive one session per output folder (question 58). | | Notebook masks (v0.4.0, PP-10c) | Step 1 has an optional masks folder; **Check folder** pairs it with `preflight(masks_folder=, mask_axes=)`, the masks folder and statements being those the run would use (`run.planned.planned_masks`, from `resolve_inputs`: given, else recorded). The **Masks** card shows each mask type (`mask_review.mask_type_rows`), the images without one and the masks' issues (`masks.issues`, and `mask_checks.mask_issues` once the roles are confirmed, as `run`). Mask axes are asked per type only where needed (`mask_review.mask_axes_questions`: `mask_not_2d`, `invalid_mask_axes`, `mask_axes_contradict_header`, and `mask_planes_mismatch` of 2-D masks, answered `YX`), with only the statements `masks.stated.mask_roles` accepts for every mask of the type; nothing is preselected or applied until **Check the masks** (`run.preflight.remasked`, which keeps the other answers). The run is given the same `masks_folder` and `mask_axes`; a masks folder not given reuses the recorded one, and **Drop the recorded masks** gives `masks_folder=False`. Step 3 is gated on masks without errors and on a pairing that matches the masks folder and statements chosen. `folder_step(show_output=False)` shows neither and pairs no masks folder. The mask step (`mask_relationships.json`) is step 3's **Describe the masks** (next row). | | Notebook mask step (v0.4.0, PP-10d) | Step 3's **Describe the masks (optional)** card. Its button is enabled when step 1's output folder holds a v2 manifest that recorded masks (`run.mask_step.recorded_masks(output_folder)`, a `RecordedMasks` from the manifest's `experiment_root`, `written_at` and `masks` keys): after a run in this session, or for an earlier run in a reopened notebook. The help line names the run it describes (output folder, when written, experiment folder, masks folder and mask types). The output folder is the one step 1 shows (default: next to the experiment folder); the experiment folder typed in step 1 only locates it, and the recorded one is read. A click runs the `masks` command's step with the run's recorded inputs (`prepare_mask_step(None, output_folder)`, the path the CLI's `masks --output` takes: experiment folder, masks folder, mask-axis statements, exclusions) and the default thresholds (the notebook offers no threshold; the command line's options change them), with a progress bar per image, then `describe_masks`, which reads the masks and writes `mask_relationships.json` holding the lock throughout. The card shows the mask types, the relationships with their counts (`mask_text.mask_type_rows`, `mask_relationship_rows`), masks of the same objects and the hierarchy (`mask_hierarchy_lines`), labelled as suggestions to check, and the path written. A refused step changes nothing: `experiment_not_recorded`, errors found since the run (an experiment or masks folder moved or removed, a mask changed; shown with their messages), no masks, masks that cannot be used (a red card), and `output_locked` (the lock card with **Remove the lock of a stopped run**). A new run clears the card. `folder_step(show_output=False)` hides the card; the pinned embedding API is unchanged (the click handler is private). | @@ -276,7 +277,7 @@ 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. 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. | +| 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 not a positive finite length in micrometres once converted (`1e308 m`, issue #93), 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); `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. | @@ -307,7 +308,7 @@ row (follow-up P3b, normalization plan N43). | `different_pixel_sizes` | Warning | The physical pixel size (X by Y) differs between images, or the z-step differs between images with more than one depth slice. Sizes are compared in micrometres by the shared rule of decision D-a (§16): two sizes are the same when they differ by at most 0.1 % of the larger, on every axis. The warning is given exactly when, on some axis, the smallest and the largest size differ, that is when some two images differ. The images are grouped by single linkage (`pixel_size_groups`) and those outside the largest group are listed. When every image is linked to the others by near-equal sizes (1.0, 1.0009, 1.0018 µm), the sizes drift, no image stands out, and the message says that none is listed for that size. Until D86 sizes were rounded to four significant digits, so 1.0004 and 1.0006 µm differed (1 and 1.001); they are now the same. | | `missing_pixel_size` | Warning | The pixel size (or the z-step of images with more than one depth slice) is known for some images and not for others. Nothing is reported when no image has it. | | `unsupported_pixel_size_unit` | Warning | An image's metadata give a pixel size or z-step (OME `PhysicalSize{X,Y,Z}`, ImageJ resolution or `spacing`, OME-Zarr level-0 scale) in a length unit that is not one of decision D-a2 (§16), for example OME `in` or ImageJ `inch`. The size is not converted and the axis is treated as uncalibrated (as before); the warning names each image, its axes and the unit as written, so the missing calibration is never silent. A unit without a size, and a plain TIFF's resolution tags (decision D-c), are not reported. The run is not refused. | -| `invalid_physical_calibration` | Warning | An image's metadata give a calibration that cannot be used (issue #94): OME-Zarr `coordinateTransformations` that are not a list or of an unsupported type, more than one scale, a scale stored in a separate file or not one number per axis, or a scale value of X, Y, Z or T that is not a positive finite number. The message names each image and what is wrong. Its sizes are unknown on those axes (never a substituted value) and recorded as uncalibrated; the warning is in the `check` and `run` issues and the report (`discovery_report.json`). An image is not named once every size the malformed calibration leaves unknown (X and Y, and Z or T with more than one plane) is stated in `calibration.csv`; a time step cannot be stated there, so a lost time step of a time series is always named. A calibration that is not given (no transformations, or no scale) is not reported. The run is not refused. | +| `invalid_physical_calibration` | Warning | An image's metadata give a calibration that cannot be used (issue #94): OME-Zarr `coordinateTransformations` that are not a list or of an unsupported type, more than one scale, a scale stored in a separate file or not one number per axis, or a scale value of X, Y, Z or T that is not a positive finite number; or, from any TIFF or OME-Zarr metadata, a size that is not a positive finite length in micrometres once converted (issue #93: `1e308 m`, `1e-320 pm`). The message names each image and what is wrong, with the axis and value. Its sizes are unknown on those axes (never a substituted value) and recorded as uncalibrated; the warning is in the `check` and `run` issues and the report (`discovery_report.json`). An image is not named once every size the malformed calibration leaves unknown (X and Y, and Z or T with more than one plane) is stated in `calibration.csv`; a time step cannot be stated there, so a lost time step of a time series is always named. A calibration that is not given (no transformations, or no scale) is not reported. The run is not refused. | | `different_channel_counts` | Error | The number of channels differs between images: the `C` dimension, or the RGB samples `S` when there is no `C`; once the axis roles are confirmed, the length of the C axis (1 without one), which replaces the header's count. The images not having the most common number are listed, with each number's image count. Every image of an experiment must have the same channels: split the others into another experiment or exclude them. | | `different_plane_counts` | Warning | The number of depth slices (`Z`), time points (`T`) or planes along any other dimension except X, Y, C and S differs between images. A missing dimension counts as 1. Channels are compared by `different_channel_counts`. | | `different_sizes` | Warning | The image width or height differs between images (TIFF and OME-Zarr). The other dimensions are compared by `different_plane_counts`. | @@ -496,7 +497,7 @@ instead of keeping copies. | Rule | Function | Detail | |---|---|---| -| Length units (D-a2) | `space_unit(text)`, `to_micrometers(value, unit)`, `SPACE_UNITS`, `UnsupportedUnitError` (`physical_sizes`) | Microscopy units only: `pm`; `Å`/`angstrom`; `nm`; `µm`/`um`/`micron(s)`/`micrometer(s)`/`micrometre(s)`; `mm`; `cm`; `m`; with each name in US and British spelling and in the plural (`nanometre`, `millimeters`, ...). Symbols of two letters or fewer are matched exactly (`Mm` is not `mm`), longer names in any case; ImageJ's `\u00B5` is decoded. `space_unit` returns the OME-NGFF name (`"micrometer"`), None when no unit is given, and refuses any other unit by name (`UnsupportedUnitError`); `to_micrometers` converts a positive finite length, None when the length or its unit is not known. This brick's readers (§2) use the same table: since D86, ImageJ and OME-NGFF sizes in British spellings (`micrometre`) are calibrated, where they were unknown before. | +| Length units (D-a2) | `space_unit(text)`, `to_micrometers(value, unit)`, `SPACE_UNITS`, `UnsupportedUnitError` (`physical_sizes`) | Microscopy units only: `pm`; `Å`/`angstrom`; `nm`; `µm`/`um`/`micron(s)`/`micrometer(s)`/`micrometre(s)`; `mm`; `cm`; `m`; with each name in US and British spelling and in the plural (`nanometre`, `millimeters`, ...). Symbols of two letters or fewer are matched exactly (`Mm` is not `mm`), longer names in any case; ImageJ's `\u00B5` is decoded. `space_unit` returns the OME-NGFF name (`"micrometer"`), None when no unit is given, and refuses any other unit by name (`UnsupportedUnitError`); `to_micrometers` converts a positive finite length, None when the length or its unit is not known; the result is always positive and finite: a length that overflows or underflows in micrometres (`1e308 m`, `1e-320 pm`), or a whole number beyond the float range, is a `ValueError` naming it (issue #93; they live in `length_units` since then, re-exported by `physical_sizes`). `PhysicalSizes.micrometers` converts with it, and is None for such a size. This brick's readers (§2) use the same table: since D86, ImageJ and OME-NGFF sizes in British spellings (`micrometre`) are calibrated, where they were unknown before. | | Plain-TIFF resolution (D-c) | `read_physical_sizes` | A TIFF's resolution tags give a pixel size only with OME or ImageJ metadata and a unit; a plain TIFF (scanner or image-editor dpi) is uncalibrated, never converted from inches or centimetres. | | Labels (D-b) | `check_labels(pixels, name)`, `LARGEST_LABEL` (`masks`) | A mask holds whole-number labels from 0 (background) to `LARGEST_LABEL` = 2,147,483,647: MorphoBricks labels are int32 in every brick. A negative label, a label above the limit, or a pixel type that is not integer or boolean is refused with a `ValueError` naming the mask and the value. `read_mask`, `piece_counts`, `pair_counts` and `scan_masks` call it. Since D86 the limit is int32; before, this brick refused only labels above 2^63 − 1 (int64), so a `uint32` or wider mask with a label of 3,000,000,000 is now refused. | | Header axes (D-d) | `header_axes(meta)`, `named_axes(meta)`, `series_axes_from_metadata(series)`, `PLACEHOLDER_AXES`, `OPEN_AXIS` (`metadata`) | The axes an image's own metadata names, as `tifffile` reads them with length-1 axes left out (`tifffile` keeps Y and X even of length 1, so an image one pixel high is `YX`): OME, ImageJ and `tifffile` "shaped" metadata (`{"axes": "CYX"}`, decision D-d), a plain 2D `YX` (or `YXS` RGB) TIFF, and an OME-Zarr image's OME-NGFF axes. None when an axis is a placeholder (`I`: a plain stack of pages; `Q`: shaped without axes, or an OME-NGFF axis of another type), or the image does not open: the file does not say which dimension is depth, time or channel. `named_axes` gives the rule position by position, with `?` (`OPEN_AXIS`) for a dimension the metadata does not name: a placeholder, and, in a plain stack of pages without axis metadata (`IYX`, `QYX`, `QQYX`; `TiffMetadata.axes_from_metadata` False, from `series_axes_from_metadata`; True, the strict reading, for a header built by hand), the `Y` and `X` too, which only say where each page's rows and columns are stored (a (Y, X, C) array written without metadata reads as `QYX`). `header_axes` gives axes exactly when nothing is open. The axis check (§2) compares confirmed roles with `named_axes`. | @@ -505,7 +506,7 @@ instead of keeping copies. | Identity (D-e) | `FOV_KEY`, `check_identity_text(value, field)`, `check_plane_index(value, axis)`, `check_identity_case(keys)`, `MISSING_TEXT`, `IdentityError` (`discovery`) | `FOV_KEY` = (`condition`, `biological_repeat`, `technical_repeat`, `position_id`), the position key of §1. `check_identity_text` accepts a label or `position_id` that is text without leading or trailing whitespace and not `""`, `NA`, `nan`, `none`, `null`, `n/a` or `` in any case (`MISSING_TEXT`); `check_plane_index` accepts None or an integer of at least 0, a Python `int` or a NumPy integer (returned as a plain `int`), and refuses everything else (bool, float, `"1"`) without converting it. Both raise `IdentityError` (a `ValueError`) naming the field and the value. Discovery applies the text rule to every folder and image name (`invalid_name`) and refuses names of one level that differ only in case anywhere in the experiment (`case_conflict`, §1). `position_id` keeps its own scope: unique within its technical repeat, ignoring case (`duplicate_position_id`). `check_identity_case` applies the same case rule to identities a brick reads from a table, one key per row in `FOV_KEY` order (or the three labels): a label of one level spelled two ways anywhere in the run, or two `position_id` values of one technical repeat equal ignoring case, raise `IdentityError` naming both spellings; the same key may repeat (several planes of one position). | | Sources of a run | `check_sources(manifest, experiment_root, relative_paths, image_axes=, identities=)`, `source_fingerprint`, `file_sha256` (`manifest`) | The checks a brick makes before using a run's table for some images (formerly CellPatcher `check_images` and MorphoFeature `histogram_fit`); from v0.4.0 file runs read through `RunInputs` (§9.1), which also checks masks. A version 1 manifest fails every image with `manifest_before_v040` alone (design v0.4 §6: every v1 run is redone). Otherwise each image must be in the manifest (`images_not_in_manifest`), still exist (`images_missing`), and be unchanged by `source_fingerprint`, its size and modification time, for an OME-Zarr folder its files' total size and newest time, so a rewritten chunk counts (`images_changed`). Its recorded `position_id` must be the one its name gives; with `identities` (`FOV_KEY` order), its labels and `position_id` must be the caller's (`identity_differs`; `position_id_not_recorded` for a manifest written before positions were recorded). With `image_axes`, the caller's axes must give each axis the role recorded for the file (`FileEntry.roles`, or the run's mapping resolved for the header in older manifests); an axis without a role (length 1) fits any letter (`axis_mapping_mismatch`). All errors, naming their images; the table's own hash is checked with `file_sha256` against `csv_sha256`. | | Masks named as images (D-i) | `mask_name_issues(leaves, mask_names=())`, `images_named_as_masks(leaves, suffixes=MASK_NAME_SUFFIXES)`, `mask_folder_part(path)`, `MASK_NAME_SUFFIXES`, `MASK_FOLDER_WORDS` (`discovery`) | The screen MorphoRelate's `init` ran (`project_image_names`), now discovery's warnings `image_named_as_mask` and `mask_folder_in_experiment` (§1) for every brick. Names are compared in lower case across the whole experiment; the longest `position_id` owns a file, an image of the same folder first. `mask_names` adds the caller's expected mask names (`cells` for `f1_cells.tif`) to `MASK_NAME_SUFFIXES`; `discover_experiment` gives none. A folder counts as a masks folder when a word of its name (`CellLabels` is `Cell` and `Labels`) is in `MASK_FOLDER_WORDS`. `images_named_as_masks` returns the `(image, owner)` pairs of that detection for the caller's own name endings (`["_cells"]`), for a brick that acts on the files itself (MorphoRelate leaves out the images named with a declared mask name). | -| Pixel sizes (D-a) | `same_pixel_size(a, b, roles=("X", "Y"))`, `pixel_size_groups(sizes, roles=)`, `PIXEL_SIZE_TOLERANCE` (`physical_sizes`) | Two pixel sizes are the same when `|a - b| <= 1e-3 * max(a, b)` on every compared axis, in micrometres; one rule for a declared size against a header and for two headers. `same_pixel_size` takes `PhysicalSizes` (converted with `micrometers`) or sizes already in micrometres, one per role, and returns None when either is unknown (no size, or a unit that cannot be converted), never True. The rule is not transitive (1.0 ≈ 1.0009 ≈ 1.0018, but 1.0 ≠ 1.0018), so there is no per-image grouping key: `pixel_size_groups` links images by single linkage, so images of two groups always differ while the extremes of one group may drift; unknown sizes are left out; the largest group first. The folder check's `different_pixel_sizes` uses both (§11). | +| Pixel sizes (D-a) | `same_pixel_size(a, b, roles=("X", "Y"))`, `pixel_size_groups(sizes, roles=)`, `PIXEL_SIZE_TOLERANCE` (`physical_sizes`) | Two pixel sizes are the same when `|a - b| <= 1e-3 * max(a, b)` on every compared axis, in micrometres; one rule for a declared size against a header and for two headers. `same_pixel_size` takes `PhysicalSizes` (converted with `micrometers`) or sizes already in micrometres, one per role, and returns None when either is unknown (no size, or a unit that cannot be converted), never True. The rule is not transitive (1.0 ≈ 1.0009 ≈ 1.0018, but 1.0 ≠ 1.0018), so there is no per-image grouping key: `pixel_size_groups` links images by single linkage, so images of two groups always differ while the extremes of one group may drift; unknown sizes are left out; the largest group first. A size that cannot be converted to a positive finite length in micrometres (`1e308 m`) is unknown, never infinite, and an infinite or NaN value is never the same as any other (issue #93; before, `inf` matched every size). The folder check's `different_pixel_sizes` uses both (§11). | | Experiment design | `read_confirmed_design(path, expected_sha256=)`, `ConfirmedDesign`, `DesignCheckError` (`design`) | The one reader of `experiment_design.csv` (§12) for a brick that pools biological repeats (formerly CellPatcher `load_experiment_design` and MorphoFeature `ExperimentDesign.read`). It takes the file as this brick writes it and fills nothing in: UTF-8 without a byte-order mark; the header exactly the §12 columns in order (a column named twice or without a name is refused naming it, as by the lenient reader, §12); one value per column in every row (an unclosed quote, a missing or extra value, or a blank line is refused); no value with outer whitespace; `control` and `confirmed` exactly `true` or `false`; counts written as whole numbers of at least 0; `schema_version` 1; `confirmed_on` empty or `YYYY-MM-DD`; `condition`, `biological_repeat` and `repeat_group` pass `check_identity_text` and do not differ only in case (`check_identity_case`); no folder twice, one `control` per condition, at least one row; every group confirmed with one folder per condition (`design_issues`); with `expected_sha256` (the manifest's `experiment_design_sha256`) the file must be that one. The SHA-256 is of the bytes parsed. `ConfirmedDesign` gives `path`, `sha256`, the `design`, read-only `repeat_groups` (`{(condition, biological_repeat): repeat_group}`, the normalization brick's input), `group_members()` (`{repeat_group: {condition: biological_repeat}}`), `conditions`, `controls` and `groups`. Refusals raise `DesignCheckError` (a `ValueError`) with its `issues`: `missing_experiment_design`, `unreadable_experiment_design`, `invalid_experiment_design`, `repeat_group_conflict`, `design_not_confirmed` or `changed_experiment_design`, each an error naming the file (or the group's folders). This brick's step 1 and `check` read the file being edited with the lenient `load_design(output_folder, repeats)` (§12), which returns `(design, changes)`: the saved design brought up to date with the folders, or the default design when there is no file. A saved file that cannot be read raises `DesignCheckError` with the one error `unreadable_experiment_design`, whose message is the same "The experiment design cannot be read: …" as here followed by the lenient reader's refusal, naming the line and column where it can (the `DesignFormatError` is its `__cause__`); no default design is returned and the file is not touched (owner decision of 31 October 2026, plan §12 question 43). **Breaking change for the next release:** up to v0.3.4 `load_design` returned a third value, `problem`, the reason as text, together with the default design. | ## 17. Named-axis copies (`convert`, v0.4.0, design v0.4 §5) diff --git a/docs/plan.md b/docs/plan.md index fe3904b6..0442843f 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -557,6 +557,13 @@ API, CLI). The notebook and CLI only call the API. header naming a column twice or a column without a name, and `RunInputs.design()` reads with `read_confirmed_design`; changing the notebook's output folder reloads the design from the new folder and asks again. +- **Before the audit: micrometre conversion and exit status (issue #93, #130 review)** + *(this PR; question 66)*: every conversion to micrometres is a positive finite length + (`length_units.to_micrometers`, moved from `physical_sizes` with the unit table, which + re-exports them); a header size that overflows or underflows is unknown and named by + `invalid_physical_calibration`, a stated one is refused, and an infinite size is never + the same as another; `check` and `design` exit with status 1 whenever they print an + error. - **Release v0.4.0 (PP-13) checklist**, for every tag of v0.4.0 including release candidates, in the release PR before the tag is pushed: - set `version` in `pyproject.toml`; @@ -1589,3 +1596,25 @@ Raised during review; to be decided before the milestones that need them: answer (the folder may still be being typed), and the design card says which. The run is gated as before, so it runs only on the new folder's confirmed design. No copy action: the user answers again, or copies the file. Contract §12; user guide §4. +66. What happens to a size that does not fit in micrometres, and what is the exit status of + a command that reports an error (issue #93, review of #130)? + *Decided (2 November 2026, design v0.4 §3: nothing guessed, nothing silent; owner to + confirm in the review):* (1) every conversion to micrometres goes through + `to_micrometers`, whose result is always a positive finite number: a length that + overflows (`1e308 m` is infinite in micrometres) or underflows (`1e-320 pm` is zero), or + a whole number beyond the float range, is a `ValueError` naming the value and unit + (before, `inf` and `0.0` were returned, and an `OverflowError` raised). + `PhysicalSizes.micrometers` uses it and is None for such a size, so `same_pixel_size` + is None and `pixel_size_groups` leaves it out (both raised `ValueError` before), and + the comparison never takes an infinite or NaN size for another (before, `inf` was "the + same" as every size, so `1e308 m` beside `0.5 µm` gave no issue at all). (2) A header + size of TIFF or OME-Zarr metadata that does so, or an OME-Zarr level-0 scale times the + image's scale that does so, is treated like the malformed calibrations of question 60 + (3): the size is None, `PhysicalSizes.invalid` gives the axis and value, and the warning + `invalid_physical_calibration` names the image (a warning, not an error, as for #94); + a stated size in `calibration.csv` or `calibrate` is refused naming the line. (3) Every + command exits with status 1 when it prints an error, and 0 otherwise (warnings never + change it): `check` exited with 0 on an experiment design it could not read, and + `design` with 0 when the design it printed had `repeat_group_conflict` or + `invalid_experiment_design`, also after writing a change. `run`, `masks`, `calibrate` + and `convert` already did. Contract §2, §9.2, §9.3, §11, §16; user guide §5, §8. diff --git a/docs/user_guide.md b/docs/user_guide.md index 61fa0e15..2780be19 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -613,8 +613,16 @@ report names the change and its effect. Other options: After writing, `run` prints the intensity checks of the table, grouped by kind, each with the channel, the image or biological repeat and the values compared, and what to do once -per kind; it draws no plots (the notebook does). Both commands exit with status 1 when an -error blocks processing; intensity warnings never do. +per kind; it draws no plots (the notebook does). + +**Exit status.** Every command (`check`, `run`, `masks`, `calibrate`, `design` and +`convert`) exits with status 1 when it prints an error, and with 0 otherwise: warnings, +intensity warnings included, never change it. So `check` exits with 1 on every error it +reports, also an experiment design it cannot read (it never replaces that file), and +`design` exits with 1 when the design it prints has a `repeat_group_conflict` or +`invalid_experiment_design`, also after a change it wrote (since v0.4.0; both exited with 0 +before). Options that cannot be read exit with 2. Scripts can therefore stop on any +non-zero status. The optional `masks` command reads the masks and tells how they relate: @@ -963,7 +971,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. | -| `invalid_physical_calibration` | Warning | An image's metadata give a pixel size, z-step or time step that cannot be used (for OME-Zarr: `coordinateTransformations` that are malformed, of an unsupported type, stored in a separate file, or a scale that is zero, negative or not a number); the message names each image and what is wrong. The image is treated as uncalibrated on those axes; no size is made up. Correct the metadata, or state the sizes with the `calibrate` command: the warning goes once every lost pixel size and z-step is stated. A lost time step cannot be stated, so its warning stays until the metadata are corrected. | +| `invalid_physical_calibration` | Warning | An image's metadata give a pixel size, z-step or time step that cannot be used (for OME-Zarr: `coordinateTransformations` that are malformed, of an unsupported type, stored in a separate file, or a scale that is zero, negative or not a number; for any image: a size too large or too small to be written in micrometres, such as `1e308 m`); the message names each image and what is wrong, with the value. The image is treated as uncalibrated on those axes; no size is made up. Correct the metadata, or state the sizes with the `calibrate` command: the warning goes once every lost pixel size and z-step is stated. A lost time step cannot be stated, so its warning stays until the metadata are corrected. | | `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. | diff --git a/pyproject.toml b/pyproject.toml index 26d9d115..27e28775 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -197,6 +197,7 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.notebook_style" = "ui" "morphobricks_preprocessing.notebook_ui" = "ui" "morphobricks_preprocessing.physical_sizes" = "io" +"morphobricks_preprocessing.length_units" = "pure" "morphobricks_preprocessing.pixel_size_checks" = "pure" "morphobricks_preprocessing.ngff_scale" = "pure" "morphobricks_preprocessing.calibration" = "pure" diff --git a/src/morphobricks_preprocessing/calibration.py b/src/morphobricks_preprocessing/calibration.py index 94e9733f..223f3ad2 100644 --- a/src/morphobricks_preprocessing/calibration.py +++ b/src/morphobricks_preprocessing/calibration.py @@ -114,7 +114,8 @@ def calibration_entry(target: str, axis: str, size: object, unit: str) -> Calibr ``"X"``, ``"Y"`` or ``"Z"`` (upper case). size A positive, finite number (or its text in plain decimal notation, such as ``"0.65"`` - or ``"1e-3"``), or None/``""`` with the unit ``"pixels"``. + or ``"1e-3"``) that is also one in micrometres (issue #93), or None/``""`` with the + unit ``"pixels"``. unit A supported length unit in any spelling :func:`.physical_sizes.space_unit` reads, or ``"pixels"``. @@ -138,8 +139,9 @@ def calibration_entry(target: str, axis: str, size: object, unit: str) -> Calibr f"a statement in pixels has no size (found {size!r}): leave the size empty" ) return CalibrationEntry(target, axis, None, PIXELS) - name = _unit(unit) - return CalibrationEntry(target, axis, _size(size), name) + name, size = _unit(unit), _size(size) + to_micrometers(size, name) # refuses one beyond micrometres: 1e308 m is inf (issue #93) + return CalibrationEntry(target, axis, size, name) def _target(text: str) -> str: @@ -181,6 +183,8 @@ def _size(value: object) -> float: size = float(value.strip()) if isinstance(value, str) else float(value) except (TypeError, ValueError): raise ValueError(f"the size {value!r} is not a number") from None + except OverflowError: # an int beyond the float range + raise ValueError(f"the size {value!r} is not a positive finite number") from None if not math.isfinite(size) or size <= 0: raise ValueError(f"the size {value!r} is not a positive finite number") return size diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index 3e00fc38..0c45ccc8 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -20,7 +20,8 @@ ``calibrate`` (:mod:`.cli_calibrate`) shows or states the pixel sizes in the output folder's ``calibration.csv`` (design v0.4 §3). ``convert`` (:mod:`.cli_convert`) saves the images whose axes are wrong or not named as named-axis OME-TIFF copies in a new folder (design v0.4 §5). -All exit with status 1 when an error blocks processing. +All exit with status 1 when they report an error (an error blocks processing) and 0 +otherwise; argparse exits with 2 on options it cannot read. """ from __future__ import annotations @@ -224,8 +225,7 @@ def _check(args: argparse.Namespace) -> int: if pre.issues: print() print(format_issues(pre.issues)) - if pre.dataset.repeats: - _print_design(args, pre) + design_refused = bool(pre.dataset.repeats) and not _print_design(args, pre) axis_errors = () if review is not None: # The suggested roles are the header's own, so they never contradict it; 'run' @@ -253,11 +253,16 @@ def _check(args: argparse.Namespace) -> int: 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 + blocked = not pre.ok or axis_errors or refusal is not None or design_refused + return 1 if blocked else 0 # every error printed exits with 1, as 'run' does -def _print_design(args: argparse.Namespace, pre: Preflight) -> None: - """Print the experiment design in the output folder; ``check`` never replaces it.""" +def _print_design(args: argparse.Namespace, pre: Preflight) -> bool: + """Print the experiment design in the output folder; False when it is refused. + + ``check`` never replaces a design. One it cannot read is printed as the error it is, + and ``check`` then exits with status 1. + """ output = Path(args.output) if args.output else default_output_folder(pre.discovery.root) try: design, _ = load_design(output, pre.dataset.repeats) @@ -268,7 +273,8 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> None: if design is None: print(f"Experiment design: {refusal.message}") print(f" Suggested action: {refusal.action}") - elif (output / DESIGN_FILENAME).exists(): + return not refusal.is_error + if (output / DESIGN_FILENAME).exists(): print(f"Experiment design ({output / DESIGN_FILENAME}):") print(design_summary(design)) else: @@ -276,6 +282,7 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> None: "Experiment design: not confirmed yet. 'check' writes experiment_design.csv, " "prefilled from the folders, to the output folder for you to confirm." ) + return True def _attach_log(path: str) -> logging.Handler: diff --git a/src/morphobricks_preprocessing/cli_design.py b/src/morphobricks_preprocessing/cli_design.py index 22277ded..bb915456 100644 --- a/src/morphobricks_preprocessing/cli_design.py +++ b/src/morphobricks_preprocessing/cli_design.py @@ -20,6 +20,7 @@ from .run import OutputLockedError from .run.design_file import edit_design from .run.lock import lock_issue +from .validation import has_errors def add_design_parser(commands: argparse._SubParsersAction) -> argparse.ArgumentParser: @@ -108,7 +109,7 @@ def design_command(args: argparse.Namespace) -> int: if problems: print() print(format_issues(problems)) - return 0 + return 1 if has_errors(problems) else 0 # an error printed is an exit status of 1 def design_instructions(path: Path, output: Path) -> list[str]: diff --git a/src/morphobricks_preprocessing/length_units.py b/src/morphobricks_preprocessing/length_units.py new file mode 100644 index 00000000..2d6a04bf --- /dev/null +++ b/src/morphobricks_preprocessing/length_units.py @@ -0,0 +1,161 @@ +"""Length units and their conversion to micrometres, as every brick reads them. + +The microscopy units of MorphoBricks decision D-a2 (:func:`space_unit`) and the shared +conversion :func:`to_micrometers`, whose result is always a positive finite length (issue +#93). Moved from :mod:`.physical_sizes`, which re-exports them; pure: no I/O. +""" + +from __future__ import annotations + +import math +import re + +# Length units in OME, ImageJ and OME-NGFF metadata, and their OME-NGFF (UDUNITS-2) names. +# The microscopy units only (MorphoBricks decision D-a2, 30 October 2026): picometre, +# angstrom, nanometre, micrometre, millimetre, centimetre and metre, by symbol and by name +# in US and British spelling. Any other unit (inch, decimetre, kilometre, ...) is not +# converted: :func:`space_unit` refuses it by name. +_SPACE_UNITS = { + "pm": "picometer", + "Å": "angstrom", # the OME symbol, LATIN CAPITAL LETTER A WITH RING ABOVE + "\u212b": "angstrom", # ANGSTROM SIGN + "å": "angstrom", # ImageJ units are compared in lower case + "nm": "nanometer", + "µm": "micrometer", + "μm": "micrometer", + "um": "micrometer", + "micron": "micrometer", + "microns": "micrometer", + "mm": "millimeter", + "cm": "centimeter", + "m": "meter", +} +for _name, _prefix in ( + ("picometer", "pico"), + ("angstrom", None), + ("nanometer", "nano"), + ("micrometer", "micro"), + ("millimeter", "milli"), + ("centimeter", "centi"), + ("meter", ""), +): + _spellings = ("angstrom",) if _prefix is None else (_prefix + "meter", _prefix + "metre") + for _spelling in _spellings: + _SPACE_UNITS[_spelling] = _name + _SPACE_UNITS[_spelling + "s"] = _name +del _name, _prefix, _spellings, _spelling + +SPACE_UNITS = tuple(sorted(set(_SPACE_UNITS.values()))) +"""The length units this brick reads and converts, as OME-NGFF (UDUNITS-2) names.""" + + +class UnsupportedUnitError(ValueError): + """Raised for a length unit that is not one of :data:`SPACE_UNITS`.""" + + +# Micrometres per OME-NGFF space unit, to compare sizes given in different units. +_MICROMETERS = { + "angstrom": 1e-4, + "picometer": 1e-6, + "nanometer": 1e-3, + "micrometer": 1.0, + "millimeter": 1e3, + "centimeter": 1e4, + "meter": 1e6, +} + + +def space_unit(text: str | None) -> str | None: + r"""Return the OME-NGFF (UDUNITS-2) name of a length unit, as every brick reads it. + + The units are those of microscopy (MorphoBricks decision D-a2): ``pm``, ``Å`` or + ``angstrom``, ``nm``, ``µm`` (also ``um``, ``micron(s)``, ``micrometer(s)``, + ``micrometre(s)``), ``mm``, ``cm`` and ``m``, each also by its name in US or British + spelling (``nanometer``, ``nanometre``, ...). ImageJ's escaped micro sign (``\u00B5m``) + is decoded. Symbols of two letters or fewer are matched exactly (``"Mm"`` is not + ``"mm"``); longer names in any case. + + Parameters + ---------- + text + The unit as written in the metadata or by the user. + + Returns + ------- + str or None + For example ``"micrometer"`` for ``"µm"``; None when no unit is given (None or blank). + + Raises + ------ + UnsupportedUnitError + For any other unit (``"inch"``, ``"decimeter"``, ``"Mm"``, ...), named in the message: + a size in it is never converted. + """ + if text is None: + return None + if not isinstance(text, str): + raise UnsupportedUnitError(f"the unit {text!r} is not text") + decoded = _decode_escapes(text).strip() + if not decoded: + return None + known = _SPACE_UNITS.get(decoded) + if known is None and len(decoded) > 2: + known = _SPACE_UNITS.get(decoded.lower()) + if known is None: + raise UnsupportedUnitError( + f"the length unit {text!r} is not supported; use pm, Å (angstrom), nm, µm (um, " + "micron), mm, cm or m, or their names (micrometer or micrometre, ...)" + ) + return known + + +def to_micrometers(value: float | None, unit: str | None) -> float | None: + """Return a length in micrometres, with the units of :func:`space_unit`. + + Parameters + ---------- + value + A positive, finite length, or None when it is not known. + unit + Its unit, as :func:`space_unit` reads it; None (or blank) when not known. + + Returns + ------- + float or None + The length in micrometres, always positive and finite; None when the value or its + unit is not known, so an unknown unit is never taken for another. + + Raises + ------ + UnsupportedUnitError + If the unit is not a supported length unit (:func:`space_unit`). + ValueError + If the value is not a positive finite number (a bool is refused), or is not one in + micrometres: ``1e308`` metres overflows to infinity and ``1e-320`` picometres + underflows to zero (issue #93). + """ + if value is None: + return None + name = space_unit(unit) + if isinstance(value, bool) or not isinstance(value, int | float): + raise ValueError(f"the length {value!r} is not a number") + try: + number = float(value) + except OverflowError: # an int beyond the float range + number = math.inf + if not math.isfinite(number) or number <= 0: + raise ValueError(f"the length {value!r} is not a positive finite number") + if name is None: + return None + converted = number * _MICROMETERS[name] + if not math.isfinite(converted) or converted <= 0: + raise ValueError( + f"the length {value!r} {name} is not a positive finite number of micrometres " + f"(it converts to {converted!r})" + ) + return converted + + +def _decode_escapes(text: str) -> str: + r"""Return ``text`` with ImageJ's ``\uXXXX`` escapes decoded (``\u00B5`` is ``µ``).""" + return re.sub(r"\\u([0-9a-fA-F]{4})", lambda m: chr(int(m.group(1), 16)), text) diff --git a/src/morphobricks_preprocessing/physical_sizes.py b/src/morphobricks_preprocessing/physical_sizes.py index bbf852b4..d094adee 100644 --- a/src/morphobricks_preprocessing/physical_sizes.py +++ b/src/morphobricks_preprocessing/physical_sizes.py @@ -21,13 +21,17 @@ import logging import math import os -import re from collections.abc import Hashable, Mapping, Sequence from dataclasses import dataclass, replace from xml.etree import ElementTree import tifffile +# The shared unit rule (contract §16), re-exported where it was before its own module. +from .length_units import _MICROMETERS, _SPACE_UNITS, _decode_escapes, to_micrometers +from .length_units import SPACE_UNITS as SPACE_UNITS +from .length_units import UnsupportedUnitError as UnsupportedUnitError +from .length_units import space_unit as space_unit from .ngff_scale import read_scale logger = logging.getLogger(__name__) @@ -36,49 +40,6 @@ """Key, in an OME-NGFF multiscales entry's ``metadata``, listing the axes (``"x"``, ...) whose ``scale`` is a placeholder 1 because their size is not known (this brick's copies).""" -# Length units in OME, ImageJ and OME-NGFF metadata, and their OME-NGFF (UDUNITS-2) names. -# The microscopy units only (MorphoBricks decision D-a2, 30 October 2026): picometre, -# angstrom, nanometre, micrometre, millimetre, centimetre and metre, by symbol and by name -# in US and British spelling. Any other unit (inch, decimetre, kilometre, ...) is not -# converted: :func:`space_unit` refuses it by name. -_SPACE_UNITS = { - "pm": "picometer", - "Å": "angstrom", # the OME symbol, LATIN CAPITAL LETTER A WITH RING ABOVE - "\u212b": "angstrom", # ANGSTROM SIGN - "å": "angstrom", # ImageJ units are compared in lower case - "nm": "nanometer", - "µm": "micrometer", - "μm": "micrometer", - "um": "micrometer", - "micron": "micrometer", - "microns": "micrometer", - "mm": "millimeter", - "cm": "centimeter", - "m": "meter", -} -for _name, _prefix in ( - ("picometer", "pico"), - ("angstrom", None), - ("nanometer", "nano"), - ("micrometer", "micro"), - ("millimeter", "milli"), - ("centimeter", "centi"), - ("meter", ""), -): - _spellings = ("angstrom",) if _prefix is None else (_prefix + "meter", _prefix + "metre") - for _spelling in _spellings: - _SPACE_UNITS[_spelling] = _name - _SPACE_UNITS[_spelling + "s"] = _name -del _name, _prefix, _spellings, _spelling - -SPACE_UNITS = tuple(sorted(set(_SPACE_UNITS.values()))) -"""The length units this brick reads and converts, as OME-NGFF (UDUNITS-2) names.""" - - -class UnsupportedUnitError(ValueError): - """Raised for a length unit that is not one of :data:`SPACE_UNITS`.""" - - _TIME_UNITS = { "ns": "nanosecond", "µs": "microsecond", @@ -91,17 +52,6 @@ class UnsupportedUnitError(ValueError): "h": "hour", } -# Micrometres per OME-NGFF space unit, to compare sizes given in different units. -_MICROMETERS = { - "angstrom": 1e-4, - "picometer": 1e-6, - "nanometer": 1e-3, - "micrometer": 1.0, - "millimeter": 1e3, - "centimeter": 1e4, - "meter": 1e6, -} - # The OME schema's unit when a PhysicalSize*Unit attribute is left out. _OME_DEFAULT_SPACE_UNIT = "µm" _OME_DEFAULT_TIME_UNIT = "s" @@ -200,94 +150,16 @@ def unit(self, role: str) -> str | None: def micrometers(self, role: str) -> float | None: """Return the size along ``"X"``, ``"Y"`` or ``"Z"`` in micrometres. - Uses the axis's own unit. None when the size or its unit is unknown, so - that sizes in different units can be compared and an unknown unit is - never taken for another. + Uses the axis's own unit, with :func:`to_micrometers`. None when the size or its + unit is unknown, so that sizes in different units can be compared and an unknown + unit is never taken for another; None too when the size is not a positive finite + length in micrometres (``1e308`` metres overflows: issue #93), never ``inf``. """ - value = {"X": self.x, "Y": self.y, "Z": self.z}[role] - factor = _MICROMETERS.get(self.unit(role) or "") - if value is None or factor is None: + value, unit = {"X": self.x, "Y": self.y, "Z": self.z}[role], self.unit(role) + try: + return to_micrometers(value, unit) if unit in _MICROMETERS else None + except ValueError: return None - return value * factor - - -def space_unit(text: str | None) -> str | None: - r"""Return the OME-NGFF (UDUNITS-2) name of a length unit, as every brick reads it. - - The units are those of microscopy (MorphoBricks decision D-a2): ``pm``, ``Å`` or - ``angstrom``, ``nm``, ``µm`` (also ``um``, ``micron(s)``, ``micrometer(s)``, - ``micrometre(s)``), ``mm``, ``cm`` and ``m``, each also by its name in US or British - spelling (``nanometer``, ``nanometre``, ...). ImageJ's escaped micro sign (``\u00B5m``) - is decoded. Symbols of two letters or fewer are matched exactly (``"Mm"`` is not - ``"mm"``); longer names in any case. - - Parameters - ---------- - text - The unit as written in the metadata or by the user. - - Returns - ------- - str or None - For example ``"micrometer"`` for ``"µm"``; None when no unit is given (None or blank). - - Raises - ------ - UnsupportedUnitError - For any other unit (``"inch"``, ``"decimeter"``, ``"Mm"``, ...), named in the message: - a size in it is never converted. - """ - if text is None: - return None - if not isinstance(text, str): - raise UnsupportedUnitError(f"the unit {text!r} is not text") - decoded = _decode_escapes(text).strip() - if not decoded: - return None - known = _SPACE_UNITS.get(decoded) - if known is None and len(decoded) > 2: - known = _SPACE_UNITS.get(decoded.lower()) - if known is None: - raise UnsupportedUnitError( - f"the length unit {text!r} is not supported; use pm, Å (angstrom), nm, µm (um, " - "micron), mm, cm or m, or their names (micrometer or micrometre, ...)" - ) - return known - - -def to_micrometers(value: float | None, unit: str | None) -> float | None: - """Return a length in micrometres, with the units of :func:`space_unit`. - - Parameters - ---------- - value - A positive, finite length, or None when it is not known. - unit - Its unit, as :func:`space_unit` reads it; None (or blank) when not known. - - Returns - ------- - float or None - The length in micrometres; None when the value or its unit is not known, so an - unknown unit is never taken for another. - - Raises - ------ - UnsupportedUnitError - If the unit is not a supported length unit (:func:`space_unit`). - ValueError - If the value is not a positive finite number (a bool is refused). - """ - if value is None: - return None - name = space_unit(unit) - if isinstance(value, bool) or not isinstance(value, int | float): - raise ValueError(f"the length {value!r} is not a number") - if not math.isfinite(value) or value <= 0: - raise ValueError(f"the length {value!r} is not a positive finite number") - if name is None: - return None - return float(value) * _MICROMETERS[name] PIXEL_SIZE_TOLERANCE = 1e-3 @@ -393,7 +265,8 @@ def root(i: int) -> int: def _same_length(a: float, b: float) -> bool: - return abs(a - b) <= PIXEL_SIZE_TOLERANCE * max(a, b) + finite = math.isfinite(a) and math.isfinite(b) # inf is never equal to anything (#93) + return finite and abs(a - b) <= PIXEL_SIZE_TOLERANCE * max(a, b) def _micrometer_values( @@ -427,12 +300,17 @@ def _sizes( ``space_unit`` is the unit of X, as before per-axis units, unless a space axis with a size has no unit: it would then fall back to X's unit, so ``space_unit`` - is None. + is None. A size that is not a positive finite length in micrometres (``1e308`` + metres overflows, issue #93) is None, with what is wrong in ``invalid``. """ + given = {"X": x, "Y": y, "Z": z} + problems = {a: _unconvertible(a, size, units[a]) for a, size in given.items()} + x, y, z = (None if problems[a] else size for a, size in given.items()) unitless = any( size is not None and units[a] is None for a, size in zip("XYZ", (x, y, z), strict=True) ) return PhysicalSizes( + invalid="; ".join(problem for problem in problems.values() if problem) or None, x=x, y=y, z=z, @@ -446,6 +324,16 @@ def _sizes( ) +def _unconvertible(axis: str, size: float | None, unit: str | None) -> str | None: + """Say why a size cannot be given in micrometres (issue #93); None when it can.""" + try: + if size is not None and unit in _MICROMETERS: + to_micrometers(size, unit) + except ValueError as exc: + return f"{axis}: {exc}" + return None + + def _unsupported( sizes: Mapping[str, float | None], written: Mapping[str, str | None] ) -> list[tuple[str, str]]: @@ -519,14 +407,19 @@ def ngff_physical_sizes(multiscale: dict, axes: str) -> PhysicalSizes: scale, problems = read_scale(first, axes, "level 0") overall, more = read_scale(multiscale, axes, "the image") problems += more - invalid = "; ".join(problems) or None - if scale is None or (more and overall is None): - return PhysicalSizes(invalid=invalid) - if overall is not None: + if scale is not None and overall is not None: scale = [ a * b if a is not None and b is not None else None for a, b in zip(scale, overall, strict=True) ] + problems += [ # two usable scales whose product is not (issue #93) + f"the scale along {letter}, level 0's times the image's, is {size!r}" + for letter, size in zip(axes, scale, strict=True) + if letter in "XYZT" and size is not None and not 0 < size < math.inf + ] + invalid = "; ".join(problems) or None + if scale is None or (more and overall is None): + return PhysicalSizes(invalid=invalid) declared = multiscale.get("axes") units: list[str | None] = [None] * len(axes) if isinstance(declared, list) and len(declared) == len(axes): @@ -554,7 +447,7 @@ def ngff_physical_sizes(multiscale: dict, axes: str) -> PhysicalSizes: sizes["T"][1], _unsupported({letter: sizes[letter][0] for letter in "XYZ"}, written), ) - return replace(found, invalid=invalid) + return replace(found, invalid="; ".join(filter(None, (invalid, found.invalid))) or None) def _tiff_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: @@ -659,11 +552,6 @@ def _imagej_text(value) -> str: # noqa: ANN001 return _decode_escapes(str(value or "")).strip().lower() -def _decode_escapes(text: str) -> str: - r"""Return ``text`` with ImageJ's ``\uXXXX`` escapes decoded (``\u00B5`` is ``µ``).""" - return re.sub(r"\\u([0-9a-fA-F]{4})", lambda m: chr(int(m.group(1), 16)), text) - - def _resolution(tag) -> float | None: # noqa: ANN001 if tag is None: return None @@ -679,6 +567,6 @@ def _number(value) -> float | None: # noqa: ANN001 return None try: number = float(value) - except (TypeError, ValueError): + except (TypeError, ValueError, OverflowError): # 10**400 overflows a float return None return number if math.isfinite(number) and number > 0 else None diff --git a/src/morphobricks_preprocessing/pixel_size_checks.py b/src/morphobricks_preprocessing/pixel_size_checks.py index dcebdf6f..7944ea42 100644 --- a/src/morphobricks_preprocessing/pixel_size_checks.py +++ b/src/morphobricks_preprocessing/pixel_size_checks.py @@ -90,7 +90,7 @@ def _lost_roles(f: TiffMetadata) -> tuple[str, ...]: def _invalid_issue( files: Sequence[TiffMetadata], by_role: Mapping[str, Mapping[str, RecordedSize]] ) -> list[ValidationIssue]: - """Warn about calibrations the metadata give but that cannot be used (issue #94). + """Warn about calibrations the metadata give but that cannot be used (issues #93, #94). Such an image is uncalibrated on the axes concerned (never a substituted value); without this warning it would look merely uncalibrated. Every image is named with what is wrong, @@ -118,8 +118,9 @@ def _invalid_issue( f"be used ({details}). The sizes it gives are not used: these images are " "uncalibrated on those axes, and no size is substituted.", list(found), - "Correct the coordinateTransformations in these images' OME-NGFF metadata, or " - "state their pixel sizes in calibration.csv (the 'calibrate' command).", + "Correct the calibration in these images' metadata (for OME-Zarr images, the " + "coordinateTransformations), or state their pixel sizes in calibration.csv (the " + "'calibrate' command).", ) ] diff --git a/tests/test_cli_exit_status.py b/tests/test_cli_exit_status.py new file mode 100644 index 00000000..16407f99 --- /dev/null +++ b/tests/test_cli_exit_status.py @@ -0,0 +1,65 @@ +"""Every command exits with status 1 when it reports an error, and 0 otherwise. + +Found in the review of #130: ``check`` printed a refused experiment design (an error) and +exited 0, while ``run`` stops on it with 1. The same mismatch was in ``design``, which +printed ``repeat_group_conflict`` and exited 0. Warnings alone never change the status. +""" + +import numpy as np +import pytest + +from morphobricks_preprocessing.cli import main +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/day2/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): + """The output folder of a first 'check', which writes the design template (status 0).""" + output = tmp_path / "checked" + assert main(["check", str(experiment), "--output", str(output)]) == 0 + assert (output / DESIGN).is_file() + return output + + +def test_check_exits_with_1_on_a_refused_design(experiment, checked, capsys): + path = checked / DESIGN + lines = path.read_text(encoding="utf-8").splitlines() + header = lines[0].split(",") + path.write_text(",".join([*header, header[0]]) + "\n" + "\n".join(lines[1:]) + "\n") + before = path.read_bytes() + capsys.readouterr() + assert main(["check", str(experiment), "--output", str(checked)]) == 1 + out = capsys.readouterr().out + assert "Experiment design: The experiment design cannot be read" in out + assert "names the column 'condition' twice" in out + assert path.read_bytes() == before # 'check' never replaces a design + # 'run' stops on the same design with 1 + assert main(["run", str(experiment), "--output", str(checked), "--axes", "YX"]) == 1 + + +def test_check_exits_with_0_on_warnings_and_an_unconfirmed_design(experiment, checked, capsys): + capsys.readouterr() + assert main(["check", str(experiment), "--output", str(checked)]) == 0 + assert "not confirmed" in capsys.readouterr().out + + +def test_design_exits_with_1_when_it_reports_a_conflict(checked, capsys): + capsys.readouterr() + assert main(["design", "--output", str(checked), "--repeat-group", "ctrl/day2=day1"]) == 1 + out = capsys.readouterr().out + assert "Error: A repeat group is one biological repeat" in out + assert "day1 has more than one folder of ctrl" in out + # the move itself is written; undoing it gives a usable design again (status 0) + assert main(["design", "--output", str(checked), "--repeat-group", "ctrl/day2=day2"]) == 0 diff --git a/tests/test_micrometre_conversion.py b/tests/test_micrometre_conversion.py new file mode 100644 index 00000000..cbf7db95 --- /dev/null +++ b/tests/test_micrometre_conversion.py @@ -0,0 +1,238 @@ +"""Issue #93: a size converted to micrometres is always a positive finite length. + +- ``to_micrometers`` refuses a value whose conversion overflows to infinity (``1e308 m``) or + underflows to zero (``1e-320 pm``), and a whole number beyond the float range, with a + ``ValueError`` naming the value and unit; valid sizes near those limits are kept. +- ``PhysicalSizes.micrometers`` goes through ``to_micrometers``: such a size is unknown + (None), never ``inf``, so ``same_pixel_size`` and ``pixel_size_groups`` never take it for + another and never raise on it. +- A header size that cannot be given in micrometres is None, with what is wrong in + ``PhysicalSizes.invalid``, so the folder check names the image and the value + (``invalid_physical_calibration``); a stated one is refused in ``calibration.csv``. +- Before the fix, ``1e308 m`` next to ``0.5 µm`` raised no issue at all (both sides were + ``inf`` in the tolerance), and next to two other sizes the preflight raised ValueError. +""" + +import math + +import numpy as np +import pytest +import tifffile + +from morphobricks_preprocessing import ( + CalibrationFormatError, + PhysicalSizes, + calibration_entry, + pixel_size_groups, + preflight, + read_calibration, + read_physical_sizes, + same_pixel_size, + to_micrometers, +) +from morphobricks_preprocessing import physical_sizes as physical_sizes_module +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.recorded_sizes import RecordedSize + +HUGE = PhysicalSizes(x=1e308, y=1e308, space_unit="meter") +HALF = PhysicalSizes(x=0.5, y=0.5, space_unit="micrometer") + + +def write_ome(path, size, unit, y_size=None): + path.parent.mkdir(parents=True, exist_ok=True) + tifffile.imwrite( + path, + np.zeros((8, 8), np.uint16), + ome=True, + photometric="minisblack", + metadata={ + "axes": "YX", + "PhysicalSizeX": size, + "PhysicalSizeXUnit": unit, + "PhysicalSizeY": size if y_size is None else y_size, + "PhysicalSizeYUnit": unit, + }, + ) + return path + + +def codes(issues): + return [issue.code for issue in issues] + + +def issue(issues, code): + (found,) = [i for i in issues if i.code == code] + return found + + +# to_micrometers ------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("value", "unit", "converts_to"), + [ + (1e308, "m", "inf"), # overflows + (1.8e302, "m", "inf"), # just above the largest float in micrometres + (1e306, "mm", "inf"), + (1e-320, "pm", "0.0"), # subnormal: underflows to zero + (5e-324, "Å", "0.0"), # the smallest subnormal + ], +) +def test_a_conversion_beyond_the_float_range_is_refused(value, unit, converts_to): + with pytest.raises(ValueError, match="micrometres") as raised: + to_micrometers(value, unit) + assert repr(value) in str(raised.value) and converts_to in str(raised.value) + + +def test_a_whole_number_beyond_the_float_range_is_a_value_error(): + # Before: OverflowError ("int too large to convert to float"), not a ValueError. + with pytest.raises(ValueError, match="not a positive finite number"): + to_micrometers(10**400, "µm") + + +@pytest.mark.parametrize( + ("value", "unit", "expected"), + [ + (1.79e302, "m", 1.79e308), # just below the largest float + (5e-324, "µm", 5e-324), # the smallest subnormal, not converted + (1e-317, "nm", 1e-320), # converts to a subnormal, still positive + (500.0, "nm", 0.5), + (0.0005, "mm", 0.5), + (5000.0, "Å", 0.5), + ], +) +def test_sizes_near_the_limits_and_across_units_are_kept(value, unit, expected): + found = to_micrometers(value, unit) + assert math.isfinite(found) and found > 0 + assert found == pytest.approx(expected, rel=1e-12) + + +# PhysicalSizes, same_pixel_size and pixel_size_groups ----------------------------------- + + +def test_an_overflowing_size_is_unknown_never_inf(): + assert HUGE.micrometers("X") is None and HUGE.micrometers("Y") is None + assert PhysicalSizes(z=1e-320, space_unit="picometer").micrometers("Z") is None + mixed = PhysicalSizes(x=500.0, y=0.0005, x_unit="nanometer", y_unit="millimeter") + assert (mixed.micrometers("X"), mixed.micrometers("Y")) == pytest.approx((0.5, 0.5)) + + +def test_an_overflowing_size_is_never_the_same_as_another(): + # Before: same_pixel_size raised ValueError ("the pixel size inf ..."). + assert same_pixel_size(HUGE, HALF) is None + assert same_pixel_size(HUGE, HUGE) is None + # Before: pixel_size_groups raised ValueError; an unknown size is left out. + assert pixel_size_groups({"huge": HUGE, "half": HALF}) == [("half",)] + assert not physical_sizes_module._same_length(0.5, math.inf) + assert not physical_sizes_module._same_length(math.inf, math.inf) + assert not physical_sizes_module._same_length(math.nan, 0.5) + + +def test_a_recorded_size_that_overflows_is_unknown(): + huge = RecordedSize("X", 1e308, "meter", "header") + assert huge.micrometers() is None + assert RecordedSize("X", 0.5, "micrometer", "header").micrometers() == 0.5 + + +# Header sizes ----------------------------------------------------------------------------- + + +def test_a_header_size_that_overflows_is_invalid_and_named(tmp_path): + sizes = read_physical_sizes(write_ome(tmp_path / "a.ome.tif", 1e308, "m", y_size=0.5e-6)) + assert sizes.x is None and sizes.micrometers("X") is None + assert sizes.micrometers("Y") == pytest.approx(0.5) + assert sizes.invalid.startswith("X: the length 1e+308 meter") + assert "inf" in sizes.invalid and "Y" not in sizes.invalid + + +def test_a_tiny_header_size_that_underflows_is_invalid(tmp_path): + sizes = read_physical_sizes(write_ome(tmp_path / "a.ome.tif", 1e-320, "pm")) + assert (sizes.x, sizes.y) == (None, None) + assert "X: the length 1e-320 picometer" in sizes.invalid and "Y: " in sizes.invalid + + +def test_usable_header_sizes_are_not_invalid(tmp_path): + sizes = read_physical_sizes(write_ome(tmp_path / "a.ome.tif", 500.0, "nm")) + assert sizes.invalid is None and sizes.micrometers("X") == pytest.approx(0.5) + + +def ngff(scale, unit, overall=None): + multiscale = { + "axes": [{"name": a, "type": "space", "unit": unit} for a in "yx"], + "datasets": [ + {"path": "0", "coordinateTransformations": [{"type": "scale", "scale": scale}]} + ], + } + if overall is not None: + multiscale["coordinateTransformations"] = [{"type": "scale", "scale": overall}] + return physical_sizes_module.ngff_physical_sizes(multiscale, "YX") + + +def test_an_ngff_scale_that_overflows_in_micrometres_is_invalid(): + sizes = ngff([0.5, 1e303], "meter") + assert sizes.y == 0.5 and sizes.x is None + assert sizes.invalid.startswith("X: the length 1e+303 meter") + + +def test_an_ngff_scale_whose_product_overflows_is_invalid(): + # Before: the product was inf and the size silently None. + sizes = ngff([1e200, 0.5], "micrometer", overall=[1e200, 1.0]) + assert sizes.y is None and sizes.x == 0.5 + assert "the scale along Y, level 0's times the image's, is inf" in sizes.invalid + tiny = ngff([1e-200, 0.5], "micrometer", overall=[1e-200, 1.0]) + assert tiny.y is None and "is 0.0" in tiny.invalid + assert ngff([0.5, 0.5], "micrometer", overall=[2.0, 2.0]).invalid is None + + +# The folder check ------------------------------------------------------------------------- + + +def test_the_folder_check_names_an_overflowing_size_never_equal(tmp_path): + # Before: no issue at all (0.5 and inf were "the same" size). + root = tmp_path / "exp" + write_ome(root / "ctrl/rep1/a.ome.tif", 0.5, "µm") + write_ome(root / "ctrl/rep1/b.ome.tif", 1e308, "m") + pre = preflight(root, tmp_path / "out") + found = issue(pre.issues, "invalid_physical_calibration") + assert found.paths == ("ctrl/rep1/b.ome.tif",) + assert "ctrl/rep1/b.ome.tif: X: the length 1e+308 meter" in found.message + assert "calibration.csv" in found.action and "coordinateTransformations" in found.action + assert issue(pre.issues, "missing_pixel_size").paths == ("ctrl/rep1/b.ome.tif",) + assert pre.ok # warnings: the image is uncalibrated, as design v0.4 §3 treats one + + +def test_the_folder_check_still_compares_the_other_sizes(tmp_path): + # Before: pixel_size_groups raised ValueError out of the preflight. + root = tmp_path / "exp" + write_ome(root / "ctrl/rep1/a.ome.tif", 0.5, "µm") + write_ome(root / "ctrl/rep1/b.ome.tif", 1e308, "m") + write_ome(root / "ctrl/rep1/c.ome.tif", 1000.0, "nm") + write_ome(root / "ctrl/rep1/d.ome.tif", 0.0005, "mm") + pre = preflight(root, tmp_path / "out") + assert {"different_pixel_sizes", "invalid_physical_calibration"} <= set(codes(pre.issues)) + different = issue(pre.issues, "different_pixel_sizes") + assert different.paths == ("ctrl/rep1/c.ome.tif",) + assert "1 x 1 micrometres" in different.message and "inf" not in different.message + + +# Stated sizes ----------------------------------------------------------------------------- + + +def test_a_stated_size_that_overflows_is_refused(tmp_path): + with pytest.raises(ValueError, match="1e\\+308 meter is not a positive finite number of mic"): + calibration_entry(".", "X", "1e308", "m") + with pytest.raises(ValueError, match="not a positive finite number"): + calibration_entry(".", "X", 10**400, "µm") + assert calibration_entry(".", "X", "1e-317", "nm").size == 1e-317 + path = tmp_path / "calibration.csv" + path.write_text("target,axis,size,unit\n.,X,1e-320,pm\n", encoding="utf-8") + with pytest.raises(CalibrationFormatError, match="line 2.*micrometres"): + read_calibration(path) + + +def test_the_calibrate_command_refuses_an_overflowing_size(tmp_path, capsys): + output = tmp_path / "out" + output.mkdir() + assert main(["calibrate", "--output", str(output), "--size", ".", "XY", "1e308", "m"]) == 1 + assert "Nothing was changed" in capsys.readouterr().out + assert not (output / "calibration.csv").exists() diff --git a/tools/code_rules_baseline.json b/tools/code_rules_baseline.json index 98f7a0fc..db27f90d 100644 --- a/tools/code_rules_baseline.json +++ b/tools/code_rules_baseline.json @@ -31,7 +31,6 @@ "src/morphobricks_preprocessing/metadata.py: # lines (max #)": 779, "src/morphobricks_preprocessing/physical_sizes.py _imagej_sizes: private function of # lines without a docstring": 27, "src/morphobricks_preprocessing/physical_sizes.py _ome_sizes: private function of # lines without a docstring": 20, - "src/morphobricks_preprocessing/physical_sizes.py: # lines (max #)": 684, "src/morphobricks_preprocessing/planes.py _layout: private function of # lines without a docstring": 45, "src/morphobricks_preprocessing/planes.py _tiff_block_plane: private function of # lines without a docstring": 14, "src/morphobricks_preprocessing/planes.py _zarr_layout: private function of # lines without a docstring": 40, From 41dc8a07487e3462b059bf9ec381fcb77e57ae35 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 20:14:56 +0000 Subject: [PATCH 2/5] test_design: check exits 1 on a design it cannot read (#130 review) The test asserted the old status 0 for a refused design that 'check' prints as an error; the new status is 1, as 'run' gives for the same design. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- tests/test_design.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_design.py b/tests/test_design.py index 474cd33a..911430b6 100644 --- a/tests/test_design.py +++ b/tests/test_design.py @@ -476,7 +476,7 @@ def test_cli_check_refuses_an_unreadable_design_without_writing(tmp_path, capsys write_experiment_design(confirm_all(default_design(repeats_of(root))), path) path.write_text(path.read_text(encoding="utf-8").replace(TODAY, "26/10/2026"), "utf-8") unreadable = path.read_bytes() - assert main(["check", str(root)]) == 0 + assert main(["check", str(root)]) == 1 # an error printed (#130 review; it was 0) out = capsys.readouterr().out assert "Experiment design: The experiment design cannot be read" in out assert "line 2, column confirmed_on" in out From 5bd98b94e38e6b7f63c362de31c4767cd02cd093 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 20:39:22 +0000 Subject: [PATCH 3/5] check reports a design's repeat_group_conflict as an error and exits 1, as run stops on it (#133 review) check described the conflict in its design summary but exited 0. The filter that design uses (design_issues without design_not_confirmed) is now cli_design.design_errors, shared by both commands. 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 | 3 ++- src/morphobricks_preprocessing/cli.py | 11 ++++++++--- src/morphobricks_preprocessing/cli_design.py | 16 +++++++++++++--- tests/test_cli_exit_status.py | 12 ++++++++++++ 6 files changed, 39 insertions(+), 9 deletions(-) diff --git a/docs/contract.md b/docs/contract.md index a0b6b877..cad48f1c 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -279,7 +279,7 @@ the bricks' tests. | 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` from before its first mask read to the write of `mask_relationships.json` (a locked folder is refused before any mask is read), 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`, `conversion` (for a tree of named-axis copies, §17: `record`, `sha256`, `source_root`; null otherwise), `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"). The notebook's **Check folder** does the same with the same functions (`run.check_step.suggested_review`, `write_check_outputs`; v0.4.0, PP-10): the report, and the template with the design card; nothing when an embedding brick hides the output folder (`folder_step(show_output=False)`). | -| Exit status (v0.4.0) | Every command (`check`, `run`, `masks`, `calibrate`, `design`, `convert`) exits with status 1 when it prints an error and 0 otherwise; warnings never change it, and options argparse cannot read exit with 2. `check` exits with 1 on every error it reports: the preflight's, the axis check's, a refused write of its outputs (`output_locked`) and an experiment design it cannot read (`unreadable_experiment_design`; found in the review of #130, it exited with 0). `design` exits with 1 when the design it prints has `repeat_group_conflict` or `invalid_experiment_design`, also after a change it wrote (it exited with 0); `design_not_confirmed` is not printed by `design` and does not count. | +| Exit status (v0.4.0) | Every command (`check`, `run`, `masks`, `calibrate`, `design`, `convert`) exits with status 1 when it prints an error and 0 otherwise; warnings never change it, and options argparse cannot read exit with 2. `check` exits with 1 on every error it reports: the preflight's, the axis check's, a refused write of its outputs (`output_locked`) and an experiment design it cannot read (`unreadable_experiment_design`; found in the review of #130, it exited with 0) or one with `repeat_group_conflict` or `invalid_experiment_design`, printed after its summary (found in the review of #133: `run` stopped on it, `check` exited with 0); `design_not_confirmed` does not count, since `check` writes the template to confirm. `design` exits with 1 when the design it prints has `repeat_group_conflict` or `invalid_experiment_design`, also after a change it wrote (it exited with 0); `design_not_confirmed` is not printed by `design` and does not count. | | Notebook (v0.4.0, PP-10) | Step 3 offers the mode (`restart`, `full`) with its effect, and lists before the run the inputs that differ from the recorded ones with their effect (`run.planned.planned_changes(given, output_folder, design=, calibration=)`, from `resolve_inputs`, `design_for_run` and `calibration_change`: the changes the run then reports). After a run it names the mode, the files replaced and the input changes, and shows the report; a stopped run says what it wrote (question 55). A refusal `output_locked` (check, run, a design answer or a stated size) shows the holder (computer, process ID, start time) and a **Remove the lock of a stopped run** button: `remove_stale_lock` only on that click, never the lock of a process still running on the same computer; on Google Drive one session per output folder (question 58). | | Notebook masks (v0.4.0, PP-10c) | Step 1 has an optional masks folder; **Check folder** pairs it with `preflight(masks_folder=, mask_axes=)`, the masks folder and statements being those the run would use (`run.planned.planned_masks`, from `resolve_inputs`: given, else recorded). The **Masks** card shows each mask type (`mask_review.mask_type_rows`), the images without one and the masks' issues (`masks.issues`, and `mask_checks.mask_issues` once the roles are confirmed, as `run`). Mask axes are asked per type only where needed (`mask_review.mask_axes_questions`: `mask_not_2d`, `invalid_mask_axes`, `mask_axes_contradict_header`, and `mask_planes_mismatch` of 2-D masks, answered `YX`), with only the statements `masks.stated.mask_roles` accepts for every mask of the type; nothing is preselected or applied until **Check the masks** (`run.preflight.remasked`, which keeps the other answers). The run is given the same `masks_folder` and `mask_axes`; a masks folder not given reuses the recorded one, and **Drop the recorded masks** gives `masks_folder=False`. Step 3 is gated on masks without errors and on a pairing that matches the masks folder and statements chosen. `folder_step(show_output=False)` shows neither and pairs no masks folder. The mask step (`mask_relationships.json`) is step 3's **Describe the masks** (next row). | | Notebook mask step (v0.4.0, PP-10d) | Step 3's **Describe the masks (optional)** card. Its button is enabled when step 1's output folder holds a v2 manifest that recorded masks (`run.mask_step.recorded_masks(output_folder)`, a `RecordedMasks` from the manifest's `experiment_root`, `written_at` and `masks` keys): after a run in this session, or for an earlier run in a reopened notebook. The help line names the run it describes (output folder, when written, experiment folder, masks folder and mask types). The output folder is the one step 1 shows (default: next to the experiment folder); the experiment folder typed in step 1 only locates it, and the recorded one is read. A click runs the `masks` command's step with the run's recorded inputs (`prepare_mask_step(None, output_folder)`, the path the CLI's `masks --output` takes: experiment folder, masks folder, mask-axis statements, exclusions) and the default thresholds (the notebook offers no threshold; the command line's options change them), with a progress bar per image, then `describe_masks`, which reads the masks and writes `mask_relationships.json` holding the lock throughout. The card shows the mask types, the relationships with their counts (`mask_text.mask_type_rows`, `mask_relationship_rows`), masks of the same objects and the hierarchy (`mask_hierarchy_lines`), labelled as suggestions to check, and the path written. A refused step changes nothing: `experiment_not_recorded`, errors found since the run (an experiment or masks folder moved or removed, a mask changed; shown with their messages), no masks, masks that cannot be used (a red card), and `output_locked` (the lock card with **Remove the lock of a stopped run**). A new run clears the card. `folder_step(show_output=False)` hides the card; the pinned embedding API is unchanged (the click handler is private). | diff --git a/docs/plan.md b/docs/plan.md index 8a3d722d..afc77df1 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1644,7 +1644,9 @@ Raised during review; to be decided before the milestones that need them: `invalid_physical_calibration` names the image (a warning, not an error, as for #94); a stated size in `calibration.csv` or `calibrate` is refused naming the line. (3) Every command exits with status 1 when it prints an error, and 0 otherwise (warnings never - change it): `check` exited with 0 on an experiment design it could not read, and + change it): `check` exited with 0 on an experiment design it could not read or that + had `repeat_group_conflict` or `invalid_experiment_design` (which `run` stops on; + review of #133), and `design` with 0 when the design it printed had `repeat_group_conflict` or `invalid_experiment_design`, also after writing a change. `run`, `masks`, `calibrate` and `convert` already did. Contract §2, §9.2, §9.3, §11, §16; user guide §5, §8. diff --git a/docs/user_guide.md b/docs/user_guide.md index 0812a257..0e91e8fe 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -625,7 +625,8 @@ per kind; it draws no plots (the notebook does). **Exit status.** Every command (`check`, `run`, `masks`, `calibrate`, `design` and `convert`) exits with status 1 when it prints an error, and with 0 otherwise: warnings, intensity warnings included, never change it. So `check` exits with 1 on every error it -reports, also an experiment design it cannot read (it never replaces that file), and +reports, also an experiment design it cannot read (it never replaces that file) or one +with a `repeat_group_conflict` or `invalid_experiment_design`, which `run` stops on; and `design` exits with 1 when the design it prints has a `repeat_group_conflict` or `invalid_experiment_design`, also after a change it wrote (since v0.4.0; both exited with 0 before). Options that cannot be read exit with 2. Scripts can therefore stop on any diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index fa506b62..18c4871b 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -34,7 +34,7 @@ from .cli_calibrate import add_calibrate_parser, calibrate_command, calibration_summary from .cli_convert import add_convert_parser, convert_command -from .cli_design import add_design_parser, design_command, design_instructions +from .cli_design import add_design_parser, design_command, design_errors, design_instructions from .cli_masks import add_masks_arguments, masks_command from .cli_run import ( add_run_arguments, @@ -274,9 +274,10 @@ def _check(args: argparse.Namespace) -> int: def _print_design(args: argparse.Namespace, pre: Preflight) -> bool: - """Print the experiment design in the output folder; False when it is refused. + """Print the experiment design in the output folder; False when it has an error. - ``check`` never replaces a design. One it cannot read is printed as the error it is, + ``check`` never replaces a design. One it cannot read, or one ``run`` would stop on + (``repeat_group_conflict``, ``invalid_experiment_design``), is printed with its error, and ``check`` then exits with status 1. """ output = Path(args.output) if args.output else default_output_folder(pre.discovery.root) @@ -293,6 +294,10 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> bool: if (output / DESIGN_FILENAME).exists(): print(f"Experiment design ({output / DESIGN_FILENAME}):") print(design_summary(design)) + problems = design_errors(design) # what 'run' would stop on, as errors + if problems: + print(format_issues(problems)) + return False else: print( "Experiment design: not confirmed yet. 'check' writes experiment_design.csv, " diff --git a/src/morphobricks_preprocessing/cli_design.py b/src/morphobricks_preprocessing/cli_design.py index bb915456..d7c65e1f 100644 --- a/src/morphobricks_preprocessing/cli_design.py +++ b/src/morphobricks_preprocessing/cli_design.py @@ -14,13 +14,13 @@ import argparse from pathlib import Path -from .design import DESIGN_FILENAME, DesignFormatError, design_issues +from .design import DESIGN_FILENAME, DesignFormatError, ExperimentDesign, 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 -from .validation import has_errors +from .validation import ValidationIssue, has_errors def add_design_parser(commands: argparse._SubParsersAction) -> argparse.ArgumentParser: @@ -105,13 +105,23 @@ def design_command(args: argparse.Namespace) -> int: 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"] + problems = design_errors(design) if problems: print() print(format_issues(problems)) return 1 if has_errors(problems) else 0 # an error printed is an exit status of 1 +def design_errors(design: ExperimentDesign) -> list[ValidationIssue]: + """Return what stops a run on this design, other than groups not confirmed yet. + + ``invalid_experiment_design`` and ``repeat_group_conflict`` (:func:`.design.design_issues`), + which ``check`` and ``design`` report as errors as ``run`` does; ``design_not_confirmed`` + is left out, since confirming is what these commands ask for. + """ + return [issue for issue in design_issues(design) if issue.code != "design_not_confirmed"] + + def design_instructions(path: Path, output: Path) -> list[str]: """Say how to fill in a design template: edit the file, or use ``design``. diff --git a/tests/test_cli_exit_status.py b/tests/test_cli_exit_status.py index 16407f99..3222f5ae 100644 --- a/tests/test_cli_exit_status.py +++ b/tests/test_cli_exit_status.py @@ -63,3 +63,15 @@ def test_design_exits_with_1_when_it_reports_a_conflict(checked, capsys): assert "day1 has more than one folder of ctrl" in out # the move itself is written; undoing it gives a usable design again (status 0) assert main(["design", "--output", str(checked), "--repeat-group", "ctrl/day2=day2"]) == 0 + + +def test_check_exits_with_1_on_a_repeat_group_conflict(experiment, checked, capsys): + # Review of #133: 'check' described the conflict in its summary but exited 0, while + # 'run' stops on it with 1. It is now reported as the error it is. + assert main(["design", "--output", str(checked), "--repeat-group", "ctrl/day2=day1"]) == 1 + capsys.readouterr() + assert main(["check", str(experiment), "--output", str(checked)]) == 1 + out = capsys.readouterr().out + assert "day1: ctrl/day1, ctrl/day2, drug/day1 (more than one folder of ctrl)" in out + assert "Error: A repeat group is one biological repeat" in out + assert main(["run", str(experiment), "--output", str(checked), "--axes", "YX"]) == 1 From 0d98fdb10dc88be6673f96b43e80d688a4a79d81 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 20:42:16 +0000 Subject: [PATCH 4/5] TIFF sizes that are inf, NaN or not positive are invalid_physical_calibration; agree the warning with its count (#133 review) An OME (PhysicalSize*, TimeIncrement) or ImageJ (spacing, finterval, a resolution of 0 with a unit) size that is itself not a positive finite number was silently None; it is now named in PhysicalSizes.invalid, the TIFF counterpart of #94. The warning said '1 image give ... their metadata ... these images are'; it now agrees with the count. Plan question 66 records the coordinator's answers for the owner to confirm. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 2 +- docs/plan.md | 14 ++-- .../physical_sizes.py | 39 +++++++-- .../pixel_size_checks.py | 13 +-- tests/test_micrometre_conversion.py | 84 ++++++++++++++++++- 5 files changed, 132 insertions(+), 20 deletions(-) diff --git a/docs/contract.md b/docs/contract.md index cad48f1c..33547617 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -320,7 +320,7 @@ row (follow-up P3b, normalization plan N43). | `different_pixel_sizes` | Warning | The physical pixel size (X by Y) differs between images, or the z-step differs between images with more than one depth slice. Sizes are compared in micrometres by the shared rule of decision D-a (§16): two sizes are the same when they differ by at most 0.1 % of the larger, on every axis. The warning is given exactly when, on some axis, the smallest and the largest size differ, that is when some two images differ. The images are grouped by single linkage (`pixel_size_groups`) and those outside the largest group are listed. When every image is linked to the others by near-equal sizes (1.0, 1.0009, 1.0018 µm), the sizes drift, no image stands out, and the message says that none is listed for that size. Until D86 sizes were rounded to four significant digits, so 1.0004 and 1.0006 µm differed (1 and 1.001); they are now the same. | | `missing_pixel_size` | Warning | The pixel size (or the z-step of images with more than one depth slice) is known for some images and not for others. Nothing is reported when no image has it. | | `unsupported_pixel_size_unit` | Warning | An image's metadata give a pixel size or z-step (OME `PhysicalSize{X,Y,Z}`, ImageJ resolution or `spacing`, OME-Zarr level-0 scale) in a length unit that is not one of decision D-a2 (§16), for example OME `in` or ImageJ `inch`. The size is not converted and the axis is treated as uncalibrated (as before); the warning names each image, its axes and the unit as written, so the missing calibration is never silent. A unit without a size, and a plain TIFF's resolution tags (decision D-c), are not reported. The run is not refused. | -| `invalid_physical_calibration` | Warning | An image's metadata give a calibration that cannot be used (issue #94): OME-Zarr `coordinateTransformations` that are not a list or of an unsupported type, more than one scale, a scale stored in a separate file or not one number per axis, or a scale value of X, Y, Z or T that is not a positive finite number; or, from any TIFF or OME-Zarr metadata, a size that is not a positive finite length in micrometres once converted (issue #93: `1e308 m`, `1e-320 pm`). The message names each image and what is wrong, with the axis and value. Its sizes are unknown on those axes (never a substituted value) and recorded as uncalibrated; the warning is in the `check` and `run` issues and the report (`discovery_report.json`). An image is not named once every size the malformed calibration leaves unknown (X and Y, and Z or T with more than one plane) is stated in `calibration.csv`; a time step cannot be stated there, so a lost time step of a time series is always named. A calibration that is not given (no transformations, or no scale) is not reported. The run is not refused. | +| `invalid_physical_calibration` | Warning | An image's metadata give a calibration that cannot be used (issue #94): OME-Zarr `coordinateTransformations` that are not a list or of an unsupported type, more than one scale, a scale stored in a separate file or not one number per axis, or a scale value of X, Y, Z or T that is not a positive finite number; or, from any TIFF or OME-Zarr metadata, a size that is not a positive finite length in micrometres once converted (issue #93: `1e308 m`, `1e-320 pm`); or a size of OME-TIFF (`PhysicalSizeX/Y/Z`, `TimeIncrement`) or ImageJ metadata (`spacing`, `finterval` and the resolution tags, with a unit) that is itself not a positive finite number (`inf`, `NaN`, zero, negative or not a number; review of #133, before silently unknown). The message names each image and what is wrong, with the axis and value. Its sizes are unknown on those axes (never a substituted value) and recorded as uncalibrated; the warning is in the `check` and `run` issues and the report (`discovery_report.json`). An image is not named once every size the malformed calibration leaves unknown (X and Y, and Z or T with more than one plane) is stated in `calibration.csv`; a time step cannot be stated there, so a lost time step of a time series is always named. A calibration that is not given (no transformations, or no scale) is not reported. The run is not refused. | | `different_channel_counts` | Error | The number of channels differs between images: the `C` dimension, or the RGB samples `S` when there is no `C`; once the axis roles are confirmed, the length of the C axis (1 without one), which replaces the header's count. The images not having the most common number are listed, with each number's image count. Every image of an experiment must have the same channels: split the others into another experiment or exclude them. | | `different_plane_counts` | Warning | The number of depth slices (`Z`), time points (`T`) or planes along any other dimension except X, Y, C and S differs between images. A missing dimension counts as 1. Channels are compared by `different_channel_counts`. | | `different_sizes` | Warning | The image width or height differs between images (TIFF and OME-Zarr). The other dimensions are compared by `different_plane_counts`. | diff --git a/docs/plan.md b/docs/plan.md index afc77df1..86178c06 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -1628,8 +1628,11 @@ Raised during review; to be decided before the milestones that need them: (it was appended to). Contract §8; user guide §5. 66. What happens to a size that does not fit in micrometres, and what is the exit status of a command that reports an error (issue #93, review of #130)? - *Decided (2 November 2026, design v0.4 §3: nothing guessed, nothing silent; owner to - confirm in the review):* (1) every conversion to micrometres goes through + *Decided, pending the owner's confirmation (2 November 2026, design v0.4 §3: nothing + guessed, nothing silent; the coordinator's answers in the review of #133, recorded for + the owner to confirm: a header size that overflows or cannot be converted is a + warning, the image uncalibrated with the reason, as the owner decided for #94; and + `design` exiting with 1 after a write that leaves a conflict is accepted):* (1) every conversion to micrometres goes through `to_micrometers`, whose result is always a positive finite number: a length that overflows (`1e308 m` is infinite in micrometres) or underflows (`1e-320 pm` is zero), or a whole number beyond the float range, is a `ValueError` naming the value and unit @@ -1639,14 +1642,15 @@ Raised during review; to be decided before the milestones that need them: the comparison never takes an infinite or NaN size for another (before, `inf` was "the same" as every size, so `1e308 m` beside `0.5 µm` gave no issue at all). (2) A header size of TIFF or OME-Zarr metadata that does so, or an OME-Zarr level-0 scale times the - image's scale that does so, is treated like the malformed calibrations of question 60 + image's scale that does so, or a TIFF OME or ImageJ size that is itself not a positive + finite number (`inf`, `NaN`, zero, negative or not a number, before silently unknown; + review of #133), is treated like the malformed calibrations of question 60 (3): the size is None, `PhysicalSizes.invalid` gives the axis and value, and the warning `invalid_physical_calibration` names the image (a warning, not an error, as for #94); a stated size in `calibration.csv` or `calibrate` is refused naming the line. (3) Every command exits with status 1 when it prints an error, and 0 otherwise (warnings never change it): `check` exited with 0 on an experiment design it could not read or that had `repeat_group_conflict` or `invalid_experiment_design` (which `run` stops on; - review of #133), and - `design` with 0 when the design it printed had `repeat_group_conflict` or + review of #133), and `design` with 0 when the design it printed had `repeat_group_conflict` or `invalid_experiment_design`, also after writing a change. `run`, `masks`, `calibrate` and `convert` already did. Contract §2, §9.2, §9.3, §11, §16; user guide §5, §8. diff --git a/src/morphobricks_preprocessing/physical_sizes.py b/src/morphobricks_preprocessing/physical_sizes.py index d094adee..49cad04b 100644 --- a/src/morphobricks_preprocessing/physical_sizes.py +++ b/src/morphobricks_preprocessing/physical_sizes.py @@ -447,7 +447,24 @@ def ngff_physical_sizes(multiscale: dict, axes: str) -> PhysicalSizes: sizes["T"][1], _unsupported({letter: sizes[letter][0] for letter in "XYZ"}, written), ) - return replace(found, invalid="; ".join(filter(None, (invalid, found.invalid))) or None) + return _with_invalid(found, [invalid]) + + +def _with_invalid(sizes: PhysicalSizes, problems: Sequence[str | None]) -> PhysicalSizes: + """Return ``sizes`` with the problems found first in ``invalid`` (issues #93, #94).""" + return replace(sizes, invalid="; ".join(filter(None, (*problems, sizes.invalid))) or None) + + +def _header_size(axis: str, value: object) -> tuple[float | None, str | None]: + """Read a size given in TIFF metadata, with what is wrong when it is not usable (#93). + + None and no problem when it is not given; None and the problem for ``inf``, ``NaN``, + zero, a negative number or text that is not a number, never silently unknown. + """ + number = _number(value) + if value is None or number is not None: + return number, None + return None, f"{axis}: the size {value!r} is not a positive finite number" def _tiff_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: @@ -470,19 +487,22 @@ def _ome_sizes(xml: str) -> PhysicalSizes: given = {axis: pixels.get(f"PhysicalSize{axis}Unit", _OME_DEFAULT_SPACE_UNIT) for axis in "XYZ"} units = {axis: _SPACE_UNITS.get(unit) for axis, unit in given.items()} time_unit = _TIME_UNITS.get(pixels.get("TimeIncrementUnit", _OME_DEFAULT_TIME_UNIT)) - sizes = {axis: _number(pixels.get(f"PhysicalSize{axis}")) for axis in "XYZ"} + read = {axis: _header_size(axis, pixels.get(f"PhysicalSize{axis}")) for axis in "XYZ"} + read["T"] = _header_size("T", pixels.get("TimeIncrement")) if time_unit else (None, None) + sizes = {axis: size for axis, (size, _) in read.items()} written = { axis: _written(unit) if units[axis] is None else None for axis, unit in given.items() } - return _sizes( + found = _sizes( sizes["X"], sizes["Y"], sizes["Z"], - _number(pixels.get("TimeIncrement")) if time_unit else None, + sizes["T"], units, time_unit, _unsupported(sizes, written), ) + return _with_invalid(found, [problem for _, problem in read.values()]) def _imagej_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: @@ -496,25 +516,28 @@ def _imagej_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: units = {axis: _imagej_unit(value) for axis, value in given.items()} page = tif.pages.first time_unit = _TIME_UNITS.get(_imagej_text(meta.get("tunit", "sec"))) + spacing = _header_size("Z", meta.get("spacing")) given_sizes = { "X": _resolution(page.tags.get("XResolution")), "Y": _resolution(page.tags.get("YResolution")), - "Z": _number(meta.get("spacing")), + "Z": spacing[0], } # Without a unit the resolution tags are a default (often 1 pixel per inch). sizes = {axis: size if units[axis] else None for axis, size in given_sizes.items()} written = { axis: _written(value) if units[axis] is None else None for axis, value in given.items() } - return _sizes( + step = _header_size("T", meta.get("finterval")) if time_unit else (None, None) + found = _sizes( sizes["X"], sizes["Y"], sizes["Z"], - _number(meta.get("finterval")) if time_unit else None, + step[0], units, time_unit, _unsupported(given_sizes, written), ) + return _with_invalid(found, [spacing[1] if units["Z"] else None, step[1]]) def _ngff_unit(value, table: dict[str, str]) -> str | None: # noqa: ANN001 @@ -557,7 +580,7 @@ def _resolution(tag) -> float | None: # noqa: ANN001 return None try: numerator, denominator = tag.value - return denominator / numerator if numerator else None + return denominator / numerator if numerator else math.inf # 0 pixels per unit (#93) except (TypeError, ValueError): return None diff --git a/src/morphobricks_preprocessing/pixel_size_checks.py b/src/morphobricks_preprocessing/pixel_size_checks.py index 7944ea42..eb56e8b5 100644 --- a/src/morphobricks_preprocessing/pixel_size_checks.py +++ b/src/morphobricks_preprocessing/pixel_size_checks.py @@ -111,16 +111,19 @@ def _invalid_issue( if not found: return [] details = "; ".join(f"{path}: {problem}" for path, problem in found.items()) + whose = agree(found, "this image's", "these images'") return [ warning( "invalid_physical_calibration", - f"{plural(len(found), 'image')} give a calibration in their metadata that cannot " - f"be used ({details}). The sizes it gives are not used: these images are " + f"{plural(len(found), 'image')} {agree(found, 'gives', 'give')} a calibration in " + f"{agree(found, 'its', 'their')} metadata that cannot be used ({details}). The " + f"sizes it gives are not used: {agree(found, 'this image is', 'these images are')} " "uncalibrated on those axes, and no size is substituted.", list(found), - "Correct the calibration in these images' metadata (for OME-Zarr images, the " - "coordinateTransformations), or state their pixel sizes in calibration.csv (the " - "'calibrate' command).", + f"Correct the calibration in {whose} metadata (for OME-Zarr images, the " + "coordinateTransformations), or state " + f"{agree(found, 'its', 'their')} pixel sizes in calibration.csv (the 'calibrate' " + "command).", ) ] diff --git a/tests/test_micrometre_conversion.py b/tests/test_micrometre_conversion.py index cbf7db95..e556d9b9 100644 --- a/tests/test_micrometre_conversion.py +++ b/tests/test_micrometre_conversion.py @@ -8,7 +8,8 @@ another and never raise on it. - A header size that cannot be given in micrometres is None, with what is wrong in ``PhysicalSizes.invalid``, so the folder check names the image and the value - (``invalid_physical_calibration``); a stated one is refused in ``calibration.csv``. + (``invalid_physical_calibration``); a stated one is refused in ``calibration.csv``. So is + a TIFF OME or ImageJ size that is itself ``inf``, NaN, zero or negative (#133 review). - Before the fix, ``1e308 m`` next to ``0.5 µm`` raised no issue at all (both sides were ``inf`` in the tolerance), and next to two other sizes the preflight raised ValueError. """ @@ -236,3 +237,84 @@ def test_the_calibrate_command_refuses_an_overflowing_size(tmp_path, capsys): assert main(["calibrate", "--output", str(output), "--size", ".", "XY", "1e308", "m"]) == 1 assert "Nothing was changed" in capsys.readouterr().out assert not (output / "calibration.csv").exists() + + +# TIFF sizes that are not positive finite numbers (#133 review: the TIFF counterpart of #94) + + +@pytest.mark.parametrize("value", ["inf", "NaN", "0", "-1", "abc"]) +def test_an_ome_tiff_size_that_is_not_positive_finite_is_invalid(tmp_path, value): + # Before: the size was silently None, so the image looked merely uncalibrated. + path = tmp_path / "a.ome.tif" + tifffile.imwrite( + path, + np.zeros((2, 8, 8), np.uint16), + ome=True, + photometric="minisblack", + metadata={ + "axes": "TYX", + "PhysicalSizeX": value, + "PhysicalSizeXUnit": "µm", + "PhysicalSizeY": 0.5, + "PhysicalSizeYUnit": "µm", + "TimeIncrement": value, + "TimeIncrementUnit": "s", + }, + ) + sizes = read_physical_sizes(path) + assert (sizes.x, sizes.y, sizes.t) == (None, 0.5, None) + assert sizes.invalid == ( + f"X: the size {value!r} is not a positive finite number; " + f"T: the size {value!r} is not a positive finite number" + ) + + +@pytest.mark.parametrize("value", [math.inf, math.nan, 0.0, -2.0]) +def test_an_imagej_size_that_is_not_positive_finite_is_invalid(tmp_path, value): + path = tmp_path / "a.tif" + tifffile.imwrite( + path, + np.zeros((2, 3, 8, 8), np.uint16), + imagej=True, + resolution=(2.0, 2.0), + metadata={"axes": "TZYX", "unit": "um", "spacing": value, "finterval": value}, + ) + sizes = read_physical_sizes(path) + assert (sizes.x, sizes.z, sizes.t) == (0.5, None, None) + assert sizes.invalid == ( + f"Z: the size {value!r} is not a positive finite number; " + f"T: the size {value!r} is not a positive finite number" + ) + + +def test_an_imagej_resolution_of_zero_is_invalid_only_with_a_unit(tmp_path): + path = tmp_path / "a.tif" + image = np.zeros((8, 8), np.uint16) + tifffile.imwrite(path, image, imagej=True, resolution=((0, 1), (2, 1)), metadata={"unit": "um"}) + sizes = read_physical_sizes(path) + assert (sizes.x, sizes.y) == (None, 0.5) + assert sizes.invalid == "X: the length inf is not a positive finite number" + # without a unit the resolution tags are a default, not a calibration + tifffile.imwrite(path, image, imagej=True, resolution=((0, 1), (2, 1))) + assert read_physical_sizes(path).invalid is None + + +@pytest.mark.parametrize( + ("bad", "says"), + [ + (["b"], ["1 image gives a calibration in its metadata", "this image is uncalibrated"]), + (["b", "c"], ["2 images give a calibration in their metadata", "these images are unca"]), + ], +) +def test_the_invalid_calibration_message_agrees_with_the_count(tmp_path, bad, says): + # #133 review: "1 image give a calibration in their metadata ... these images are". + root = tmp_path / "exp" + write_ome(root / "ctrl/rep1/a.ome.tif", 0.5, "µm") + for name in bad: + write_ome(root / f"ctrl/rep1/{name}.ome.tif", "inf", "µm") + found = issue(preflight(root, tmp_path / "out").issues, "invalid_physical_calibration") + assert found.paths == tuple(f"ctrl/rep1/{name}.ome.tif" for name in bad) + assert "X: the size 'inf' is not a positive finite number" in found.message + for text in says: + assert text in found.message + found.action + assert ("this image's metadata" if len(bad) == 1 else "these images' metadata") in found.action From 88f3202190643869e2e37f4bfeb63273662f6fdf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 20:43:43 +0000 Subject: [PATCH 5/5] =?UTF-8?q?Review=20#133=20(=C2=A79):=20docstrings=20f?= =?UTF-8?q?or=20the=20rewritten=20=5Fome=5Fsizes=20and=20=5Fimagej=5Fsizes?= =?UTF-8?q?;=20shrink=20the=20baseline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- src/morphobricks_preprocessing/physical_sizes.py | 2 ++ tools/code_rules_baseline.json | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/morphobricks_preprocessing/physical_sizes.py b/src/morphobricks_preprocessing/physical_sizes.py index 49cad04b..20328fc8 100644 --- a/src/morphobricks_preprocessing/physical_sizes.py +++ b/src/morphobricks_preprocessing/physical_sizes.py @@ -480,6 +480,7 @@ def _tiff_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: def _ome_sizes(xml: str) -> PhysicalSizes: + """Read the sizes of an OME-XML ``Pixels`` element, naming the unusable ones (#93).""" root = ElementTree.fromstring(xml) pixels = next((e for e in root.iter() if e.tag.rsplit("}", 1)[-1] == "Pixels"), None) if pixels is None: @@ -506,6 +507,7 @@ def _ome_sizes(xml: str) -> PhysicalSizes: def _imagej_sizes(tif: tifffile.TiffFile) -> PhysicalSizes: + """Read the sizes of an ImageJ TIFF, naming the unusable ones of a known unit (#93).""" meta = tif.imagej_metadata or {} # ImageJ writes yunit and zunit only when they differ from the X unit. given = { diff --git a/tools/code_rules_baseline.json b/tools/code_rules_baseline.json index 03299f89..8c7cd390 100644 --- a/tools/code_rules_baseline.json +++ b/tools/code_rules_baseline.json @@ -29,8 +29,6 @@ "src/morphobricks_preprocessing/metadata.py _check_plane_counts: private function of # lines without a docstring": 27, "src/morphobricks_preprocessing/metadata.py check_metadata: # executable lines (max #)": 97, "src/morphobricks_preprocessing/metadata.py: # lines (max #)": 779, - "src/morphobricks_preprocessing/physical_sizes.py _imagej_sizes: private function of # lines without a docstring": 27, - "src/morphobricks_preprocessing/physical_sizes.py _ome_sizes: private function of # lines without a docstring": 20, "src/morphobricks_preprocessing/planes.py _layout: private function of # lines without a docstring": 45, "src/morphobricks_preprocessing/planes.py _tiff_block_plane: private function of # lines without a docstring": 14, "src/morphobricks_preprocessing/planes.py _zarr_layout: private function of # lines without a docstring": 40,