diff --git a/AGENTS.md b/AGENTS.md index e313882a..a740bfd5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -441,7 +441,7 @@ Technical details go to `logging` (shown in the notebook's "Technical details" p | System files (Q5) | Ignored silently; `single` check and sibling uniqueness are case-insensitive | | Too-deep folders (Q7) | Error | | Folders named `single` (Q23) | Accepted as the level's value `single` when lower case and the only folder in its place (MorphoBricks one-member level, MorphoRelate M3); another case or other folders beside it stay `reserved_folder_name` | -| Channel names (Q1) | Manifest only (from OME), warning `channel_names_differ`; CSV stays at schema 1 | +| Channel names (Q1) | Manifest only (from OME); `channel_names_differ` was a warning, an error since issue #37 (channels are matched by number, no override); names trimmed of leading/trailing whitespace at read time for every image (manifest `channel_names_trimmed`), inner whitespace compared exactly; copies leave unnamed channels unlabelled; CSV stays at schema 1 | | Plane coordinates (Q17) | `z`, `c`, `t` always written (0 for absent axes), `plane_id` always `::z=…,c=…,t=…`; CSV schema 2, version-1 tables recomputed on the next run. Superseded for the CSV by Q27: rows have `z`, `t` and channels in columns | | Channels across columns (Q27, P3b) | CSV schema 3 with the brick's writer: one row per XYC plane, per-channel CellProfiler-style columns, `plane_id` `::z=…,t=…`. Channels read one at a time (owner's choice); memory estimate adds the plane's finished bin counts. `different_channel_counts` error before any read (headers in step 1, confirmed roles in step 2); `different_plane_counts` without channels. Schema 1/2 tables recomputed; manifest version 1 records schema 3; step 4 output unchanged. Brick first pinned by commit `0e53e67`, now by its tag v0.2.0 (Q28) | | OME-NGFF labels (Q18) | Read-only helpers `ngff_label_names`/`open_ngff_labels` for later bricks (CellPatcher masks); label images are never analysed and the outputs do not change | diff --git a/docs/contract.md b/docs/contract.md index f90fe6e7..5f9b6630 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -52,7 +52,7 @@ channel order; an image without a C axis has one channel. | OME-Zarr images | Only level 0 (the first dataset of the first multiscales image) is read. A store with more than one image (several multiscales, a bioformats2raw folder with several series, a plate, a well with several fields) is an error (`multiple_series`). | | 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 a warning. | +| 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). Ångström (`Å`) and ImageJ's escaped micro sign (`\u00B5m`) are recognised. Nothing is guessed: what the metadata does not give is None; a scale in a separate file (`path`) or not one number per axis gives no sizes; an OME-Zarr scale without a unit is kept with the unit 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). | @@ -118,8 +118,8 @@ method is an offset and a scale, `(pixels − offset) / scale`. | Severity | Situations | Effect | |---|---|---| -| Error | Experiment design not confirmed (`design_not_confirmed`) or a repeat group holding two folders of one condition (`repeat_group_conflict`), when a design is used (§12); image type RGB for images that are not three channels of an integer type (`rgb_not_possible`, §13); mixed dtypes; images with different numbers of channels (`different_channel_counts`, §2); unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images, or converting TIFF images to OME-Zarr, without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles; every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single` in another case, or beside other folders (§1); sibling folders differing only in case; images in one leaf folder with the same position identifier (§1); folder below the technical-repeat level; output folder inside the experiment; no usable planes; when masks are given, a masks folder missing or inside the experiment, mask names differing only in case, two masks of one type for an image, a mask whose name could belong to two images or types, or a mask that is unreadable, not 2D, of another size than its image or not of integers or booleans (§15) | Processing is blocked | -| Warning | Intensity checks of the written table (`image_intensity_outlier`, `repeat_intensity_shift`, `saturated_image`, `no_intensity_spread`, §14), reported apart from the run's issues; files that are not images; empty folders; images outside leaf folders; images excluded by the user, or excluded paths that match no image; channel names differing between files; non-finite pixels; different image widths or heights; different numbers of depth slices, time points or other planes; physical pixel sizes that differ or are known for only some images; unbalanced numbers of images; biological repeats missing from some conditions (§11); planes without finite pixels while others remain; channels whose central intensity range covers too few histogram bins (`narrow_histogram_range`, §4); planes over the memory limit that are read in blocks; images without a mask of a type other images have, masks matching no analysed image, files outside the mask-type folders (§15) | Processing continues; the item is reported | +| Error | Experiment design not confirmed (`design_not_confirmed`) or a repeat group holding two folders of one condition (`repeat_group_conflict`), when a design is used (§12); image type RGB for images that are not three channels of an integer type (`rgb_not_possible`, §13); mixed dtypes; images with different numbers of channels (`different_channel_counts`, §2) or different channel names at the same channel number (`channel_names_differ`, §2); unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images, or converting TIFF images to OME-Zarr, without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles; every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single` in another case, or beside other folders (§1); sibling folders differing only in case; images in one leaf folder with the same position identifier (§1); folder below the technical-repeat level; output folder inside the experiment; no usable planes; when masks are given, a masks folder missing or inside the experiment, mask names differing only in case, two masks of one type for an image, a mask whose name could belong to two images or types, or a mask that is unreadable, not 2D, of another size than its image or not of integers or booleans (§15) | Processing is blocked | +| Warning | Intensity checks of the written table (`image_intensity_outlier`, `repeat_intensity_shift`, `saturated_image`, `no_intensity_spread`, §14), reported apart from the run's issues; files that are not images; empty folders; images outside leaf folders; images excluded by the user, or excluded paths that match no image; non-finite pixels; different image widths or heights; different numbers of depth slices, time points or other planes; physical pixel sizes that differ or are known for only some images; unbalanced numbers of images; biological repeats missing from some conditions (§11); planes without finite pixels while others remain; channels whose central intensity range covers too few histogram bins (`narrow_histogram_range`, §4); planes over the memory limit that are read in blocks; images without a mask of a type other images have, masks matching no analysed image, files outside the mask-type folders (§15) | Processing continues; the item is reported | Every message states what happened in plain language, lists the affected paths, and suggests an action. Technical details go to the diagnostic log. @@ -158,7 +158,7 @@ file's schema or statistical interpretation. | `settings` | `bin_count`, `memory_limit` (bytes), `missing_as_singleton`, `exclude` (sorted relative paths), `convert_tiffs` (whether TIFF images are converted to OME-Zarr and processed from the copies; false when absent), `conversion` (`chunk_size`, `compression`, `compression_level`, `pyramid_levels` — null for automatic; defaults when absent), `histogram` (`HistogramSettings.to_dict()` of the normalization brick: `library_version`, `histogram_range`, `robust_quantiles`, `min_central_bins`, §4; the default `minmax` range when absent, in manifests written before follow-up P6). | | `axis_mapping` | `axes` (file axis labels) and `roles` (`X`, `Y`, `Z`, `C`, `T` or null per axis). | | `labels` | One entry per leaf folder: `folder` (relative path, `.` for the root) and its three labels. | -| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count` (its rows in the table: XYC planes), `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent), `position_id` (the image's position identifier, §1; null when absent, in manifests written before it). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | +| `files` | One entry per analysed file, sorted by path: `relative_path`, `size` (bytes), `mtime_ns`, the three labels, `axes`, `singleton_roles`, `plane_count` (its rows in the table: XYC planes), `channel_names` (list, null for unnamed channels; empty if none in metadata), `significant_bits` (null if not in metadata), `ome_zarr_path` (for a TIFF converted to OME-Zarr, the copy relative to the output folder; null otherwise or when the key is absent), `read_in_blocks` (true when the image's planes are over the memory limit and are read in blocks; information only, false when absent), `position_id` (the image's position identifier, §1; null when absent, in manifests written before it), `channel_names_trimmed` (true when leading or trailing whitespace was removed from the image's channel names as they were read, §2; `channel_names` holds the trimmed names; false when absent). For an OME-Zarr image, `size` is the total size of its files and `mtime_ns` the time of its newest file. | | `csv_sha256` | SHA-256 of the table written with this manifest. | | `experiment_design_sha256` | SHA-256 of the `experiment_design.csv` written with this run (§12), so later bricks can check that the design they read is the one used; null when the run had no design, or absent in manifests written before it. Optional key: the manifest stays version 1. | | `masks` | The masks paired with the images (§15), when the run was given a masks folder: `folder` (absolute path of the masks folder, null when only OME-Zarr label images were used), `types` (sorted mask-type names) and `files` (one entry per image and mask type, sorted by image then type: `image`, `type`, `path`, `source` `folder` or `ome-zarr`). Absent when the run had no masks. Optional key: the manifest stays version 1, and the table does not depend on the masks. | @@ -168,7 +168,10 @@ file's schema or statistical interpretation. | `narrow_histogram_ranges` | The channel planes whose range check found the central range too narrow (§4), sorted by plane and channel: `relative_path`, `plane_id`, `channel` and the fields of the normalization brick's `RangeCheck` (`histogram_range`, `central_percentiles`, `central_bins`, `robust_quantiles`, `robust_limits`, `outside_robust_count`, `robust_central_bins`, `min_central_bins`). Empty when there are none; absent in manifests written before follow-up P6. Optional key: the manifest stays version 1. | | `versions` | Versions of `morphobricks-preprocessing`, `morphobricks-normalization` (which computes the histograms and writes the CSV; absent in manifests written before it), `numpy`, `tifffile` and Python. Information only: a different version never causes a rebuild. | -Channel names and significant bits come from OME metadata and are for information only. +Channel names and significant bits come from OME metadata. Significant bits are for +information only; channel names must agree between images at each channel number (§2), and +later bricks that name channels should check their names against `channel_names` when it +is not empty. ## 10. Converting TIFF images to OME-Zarr (optional) @@ -193,7 +196,7 @@ presentation does not change the conversion rules below. | Further levels | 2 x 2 block means rounded to the pixel type; automatic (default: until a plane fits in 512 x 512, at most 8 levels in all), none, or 1 to 7 levels; for viewing only | | Large planes | A plane read in blocks is written block by block; its further levels are then built from the stored level above, in regions of even size, and equal those of a plane written whole | | Scale | Physical pixel sizes, z-step and time step from OME or ImageJ metadata, with OME-NGFF units, each axis with its own unit (OME `PhysicalSizeXUnit`, `PhysicalSizeYUnit`, `PhysicalSizeZUnit`, micrometres when left out; ImageJ `unit`, `yunit`, `zunit`); 1 when unknown | -| Channels | `omero` block with channel names from metadata (or `Channel `) and each channel's observed minimum and maximum as display window | +| Channels | `omero` block with each channel's observed minimum and maximum as display window, and its name from metadata as `label`, as it is read (§2); an unnamed channel has no `label` (copies written earlier with the placeholder `Channel ` count as changed and are converted again), so a copy reads back with its source's channel names (§2) | | Updates | Copies of unchanged images are kept, of new or changed images rewritten, and of images no longer analysed removed; a failed run leaves the earlier copies as they were (see §9 for publication) | | Switching | Turning conversion on or off recomputes every TIFF row; turning it off removes the copies the manifest recorded | | Missing copy | A converted TIFF whose copy was deleted counts as changed and is converted again | @@ -223,7 +226,7 @@ row (follow-up P3b, normalization plan N43). | Counts | Only images to analyse count: excluded images are left out, and a folder whose images are all excluded is not a biological repeat. A biological repeat with images directly in its folder has no technical-repeat folders (count 0). | | Balance table | Conditions as columns, repeat groups as rows (§12; by default the biological-repeat folder name, `single` when there are no biological-repeat folders); each cell holds the number of images and of technical-repeat folders. | | Pixel sizes | TIFF: OME `PhysicalSizeX`, `PhysicalSizeY`, `PhysicalSizeZ` with their units (micrometres when not given), or ImageJ resolution tags and `spacing` with the ImageJ unit (unknown without a unit). OME-Zarr: the level-0 scale (`coordinateTransformations` of the first dataset, times the image's own scale when given) with the axis units; a scale without a unit is unknown. Sizes are never guessed. | -| Panel | The notebook shows the balance table with the issues `mixed_dtypes`, `inconsistent_dimensions`, `missing_axes`, `inconsistent_mapping`, `different_sizes`, `different_channel_counts`, `different_plane_counts`, `channel_names_differ`, `different_pixel_sizes`, `missing_pixel_size`, `unbalanced_images` and `repeat_missing_conditions`. The command line `check` prints the balance table and every issue; `run` reports the same warnings, and stops on `different_channel_counts`. | +| Panel | The notebook shows the balance table with the issues `mixed_dtypes`, `inconsistent_dimensions`, `missing_axes`, `inconsistent_mapping`, `different_sizes`, `different_channel_counts`, `different_plane_counts`, `channel_names_differ`, `different_pixel_sizes`, `missing_pixel_size`, `unbalanced_images` and `repeat_missing_conditions`. The command line `check` prints the balance table and every issue; `run` reports the same warnings, and stops on `different_channel_counts` and `channel_names_differ`. | | Outputs | Unchanged: the checks write nothing, and `plane_histograms.csv` and `run_manifest.json` do not change. | ## 12. Experiment design (`experiment_design.csv`, schema version 1) diff --git a/docs/plan.md b/docs/plan.md index d6f0069e..3a18267f 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -184,7 +184,7 @@ a run manifest (`run_manifest.json`) records the confirmed labels and axis mappi settings, package versions and each source file's size and modification time. It also records each file's channel names and significant bits when metadata provides them, for information only; channel names that differ between files at the same channel index are -reported as a warning. +an error (question 1, revised after the data-flow audit, issue #37). ## 7. Incremental updates @@ -406,6 +406,17 @@ Raised during review; to be decided before the milestones that need them: *Decided (October 2026): record channel names per file in the run manifest, not in the CSV, and warn when the names at the same channel index differ between files. The CSV stays at schema version 1.* + *Revised (October 2026, data-flow audit, issue #37): names that differ at the same + channel index are the error `channel_names_differ`, because channels are matched by + number and a different stain would be pooled and scaled as the same channel. Only named + channels are compared; there is no override, so such images are exported again or + excluded. Owner decision (October 2026, review of #42): leading and trailing whitespace + is removed from channel names at read time, for every image (sources and this brick's + copies), and recorded in the manifest (`channel_names_trimmed`); whitespace inside a + name is still compared exactly (`DA PI` and `DAPI` differ, and the message quotes names + that differ only in whitespace). OME-Zarr copies + leave unnamed channels without a label instead of the placeholder `Channel `, so a + copy never names a channel its source does not.* 2. Multi-series and pyramidal TIFFs: use only full-resolution level 0, and block files with several positions? *Decided (October 2026): yes. Pyramidal files are read at full-resolution level 0 only; diff --git a/docs/user_guide.md b/docs/user_guide.md index 10ff4bb3..a4849e65 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -467,7 +467,7 @@ The default output folder is next to the experiment folder: `ome_zarr/control/bio_rep1/tech_rep1/img_01.ome.zarr`. The full-resolution level holds exactly the source pixel values; smaller levels are averages for fast viewing (for example in napari with the `napari-ome-zarr` plugin). Pixel sizes and channel names are - copied from the metadata when present, each axis with its own unit. Copies of unchanged + copied from the metadata when present (an unnamed channel stays unnamed), each axis with its own unit. Copies of unchanged images are kept on later runs, copies of deleted or excluded images are removed. Copies written by an earlier version with the X unit on every axis are converted again once, with their own units; the table does not change. Switching back to reading the @@ -556,7 +556,7 @@ continues. | `repeat_intensity_shift` | Warning (intensity check) | A biological repeat is brighter or darker than the other repeats of its condition. Check whether it was prepared or acquired differently; normalising per biological repeat in later bricks removes such differences. | | `saturated_image` | Warning (intensity check) | More than the limit (default 1 %) of an image's pixels in a channel are at the largest value of its pixel type, so their true intensity is lost. Check the exposure, gain or laser power. | | `no_intensity_spread` | Warning (intensity check) | An image's channel has the same 1st and 99.8th percentile (empty or blank), so it cannot be normalised on its own. Check the channel, or exclude the image. | -| `channel_names_differ` | Warning | Channel names differ between images at the same position. Check the channel order. | +| `channel_names_differ` | Error | Channel names differ between images at the same channel number, so different stains would be pooled as one channel. Spaces at the start or end of a name are ignored (and noted in the manifest); otherwise names must match exactly, and names that differ only in inner spaces are quoted and reported as such. Check the channel order; export the images again with the same channel order and names, or exclude the images whose channels differ. | | `non_finite_pixels` | Warning | NaN or infinite pixels are left out of the histograms and counted. | | `no_finite_pixels` | Warning | Some planes have a channel without finite pixels; those channels are left out of group statistics. | | `excluded_images`, `excluded_path_not_found` | Warning | Images excluded as requested, or an excluded path that matches no image. | diff --git a/notebooks/MorphoBricks_Preprocessing.ipynb b/notebooks/MorphoBricks_Preprocessing.ipynb index 3c49c601..82dda79b 100644 --- a/notebooks/MorphoBricks_Preprocessing.ipynb +++ b/notebooks/MorphoBricks_Preprocessing.ipynb @@ -241,7 +241,8 @@ " biological repeat that a condition does not have. Below it, **Show consistency warnings**\n", " lists differences between the images: pixel type, dimensions, sizes, numbers of channels,\n", " slices or time points, channel names and pixel sizes. Warnings never stop you; each says\n", - " what to check. Different numbers of channels are an error: every image needs the same channels.\n", + " what to check. Different numbers of channels, or different channel names at the same channel\n", + " number, are errors: every image needs the same channels in the same order.\n", "\n", "> 💡 **Tip:** to change a label, rename the folder and click **Check folder** again.\n", ">\n", diff --git a/src/morphobricks_preprocessing/manifest.py b/src/morphobricks_preprocessing/manifest.py index 273e60c6..eca87537 100644 --- a/src/morphobricks_preprocessing/manifest.py +++ b/src/morphobricks_preprocessing/manifest.py @@ -116,6 +116,9 @@ class FileEntry: ome_zarr_path For a TIFF converted to OME-Zarr, the converted copy relative to the output folder (its rows are computed from it); otherwise None. + channel_names_trimmed + True when leading or trailing whitespace was removed from the image's + channel names as they were read (names are recorded trimmed). read_in_blocks True when the image's planes are too large for the memory limit and are read in blocks. For information only: the rows are the same either way. @@ -137,6 +140,7 @@ class FileEntry: ome_zarr_path: str | None = None read_in_blocks: bool = False position_id: str | None = None + channel_names_trimmed: bool = False def same_source(self, other: FileEntry) -> bool: """Return True if ``other`` describes the same unchanged file, read the same way.""" @@ -403,6 +407,7 @@ def file_entry_for( significant_bits: int | None = None, ome_zarr_path: str | None = None, read_in_blocks: bool = False, + channel_names_trimmed: bool = False, ) -> FileEntry: """Describe a source image as it is on disk now, with its position identifier.""" size, mtime_ns = source_fingerprint(Path(root) / relative_path) @@ -419,6 +424,7 @@ def file_entry_for( ome_zarr_path=ome_zarr_path, read_in_blocks=read_in_blocks, position_id=position_id(relative_path), + channel_names_trimmed=channel_names_trimmed, ) @@ -524,6 +530,7 @@ def _file_dict(entry: FileEntry) -> dict: "ome_zarr_path": entry.ome_zarr_path, "read_in_blocks": entry.read_in_blocks, "position_id": entry.position_id, + "channel_names_trimmed": entry.channel_names_trimmed, } @@ -546,6 +553,7 @@ def _file_entry(data: dict) -> FileEntry: ome_zarr_path=None if data.get("ome_zarr_path") is None else str(data["ome_zarr_path"]), read_in_blocks=bool(data.get("read_in_blocks", False)), position_id=None if data.get("position_id") is None else str(data["position_id"]), + channel_names_trimmed=bool(data.get("channel_names_trimmed", False)), ) diff --git a/src/morphobricks_preprocessing/metadata.py b/src/morphobricks_preprocessing/metadata.py index e6cfc84a..6877a418 100644 --- a/src/morphobricks_preprocessing/metadata.py +++ b/src/morphobricks_preprocessing/metadata.py @@ -29,7 +29,13 @@ from .discovery import DiscoveryResult from .physical_sizes import PhysicalSizes, _tiff_sizes from .validation import ValidationIssue, error, has_errors, plural, warning -from .zarr_input import NgffError, ZarrUnavailableError, is_zarr_name, open_ngff +from .zarr_input import ( + NgffError, + ZarrUnavailableError, + is_zarr_name, + open_ngff, + read_channel_names, +) logger = logging.getLogger(__name__) @@ -68,6 +74,11 @@ class TiffMetadata: channel_names Channel names from OME metadata, by channel index; None for a channel without a name. Empty when the file has no OME channel information. + Leading and trailing whitespace is removed from every name; whitespace + inside a name is kept and compared exactly. + channel_names_trimmed + True when leading or trailing whitespace was removed from a channel name + (recorded in the run manifest). significant_bits Bits per pixel that carry data (e.g. 12 for a 12-bit camera in 16-bit files), from OME metadata; None when not given. For information only. @@ -98,6 +109,7 @@ class TiffMetadata: kind: str = TIFF physical_sizes: PhysicalSizes = PhysicalSizes() rgb_samples: bool = False + channel_names_trimmed: bool = False def length(self, axis: str) -> int: """Return the length of the dimension labelled ``axis``, 1 if there is none.""" @@ -222,6 +234,7 @@ def inspect_ome_zarr(path: str | os.PathLike, relative_path: str | None = None) series_count=image.image_count, page_count=0, channel_names=image.channel_names, + channel_names_trimmed=image.channel_names_trimmed, kind=OME_ZARR, physical_sizes=image.physical_sizes, ) @@ -287,12 +300,12 @@ def check_metadata(files: Iterable[TiffMetadata]) -> list[ValidationIssue]: unsupported pixel types, more than one pixel type, TIFF files whose dimensions are arranged differently, and images with different numbers of channels (``different_channel_counts``; the C axis, or the RGB samples - without one, until the axis roles are confirmed). OME-Zarr images declare their axes, + without one, until the axis roles are confirmed), and channel names that differ at the + same channel (``channel_names_differ``). OME-Zarr images declare their axes, so their arrangement is checked by role during the axis review instead. Reports, as warnings: images of different widths or heights - (``different_sizes``), channel names that differ at the same channel - (``channel_names_differ``), different numbers of depth slices, time points + (``different_sizes``), different numbers of depth slices, time points or planes along another dimension (``different_plane_counts``), and physical pixel sizes that differ (``different_pixel_sizes``) or are known for only some images (``missing_pixel_size``). @@ -423,12 +436,13 @@ def _ome_details(tif: tifffile.TiffFile) -> dict: pixels = next((e for e in root.iter() if _local_name(e.tag) == "Pixels"), None) if pixels is None: return {} - names = tuple( - channel.get("Name") or None for channel in pixels if _local_name(channel.tag) == "Channel" + names, trimmed = read_channel_names( + channel.get("Name") for channel in pixels if _local_name(channel.tag) == "Channel" ) bits = pixels.get("SignificantBits") return { - "channel_names": names if any(names) else (), + "channel_names": names, + "channel_names_trimmed": trimmed, "significant_bits": int(bits) if bits and bits.isdigit() else None, } @@ -455,21 +469,33 @@ def _check_channel_names(files: list[TiffMetadata]) -> list[ValidationIssue]: descriptions: list[str] = [] for index in range(max(len(f.channel_names) for f in named)): at_index = [f for f in named if index < len(f.channel_names) and f.channel_names[index]] - if len({f.channel_names[index] for f in at_index}) > 1: - summary, minority = _breakdown(at_index, lambda f, i=index: f.channel_names[i], str) - descriptions.append(f"channel {index}: {summary}") + names = {f.channel_names[index] for f in at_index} + if len(names) > 1: + # Names are compared exactly. When they differ only in whitespace, quote them + # and say so: the difference would otherwise be invisible in the message. + only_spaces = len({" ".join(name.split()) for name in names}) == 1 + summary, minority = _breakdown( + at_index, + lambda f, i=index: f.channel_names[i], + (lambda name: f'"{name}"') if only_spaces else str, + ) + note = " (the names differ only in spaces or other whitespace)" if only_spaces else "" + descriptions.append(f"channel {index}: {summary}{note}") differing.extend(minority) if not differing: return [] + # An error (issue #37): channels are matched by number, so a different stain at the + # same number would be pooled and scaled as one channel. return [ - warning( + error( "channel_names_differ", "The images name their channels differently at the same channel position (" + "; ".join(descriptions) - + "). Planes with the same channel number may show different stains.", + + "). Channels are matched by their number, so different stains would be pooled " + "as one channel.", sorted(set(differing)), - "No action needed if the names only differ in spelling. Otherwise check the " - "channel order of these images before comparing channels.", + "Check the channel order of these images. Export them again with the same channel " + "order and names, or exclude the images whose channels differ.", ) ] diff --git a/src/morphobricks_preprocessing/run.py b/src/morphobricks_preprocessing/run.py index 4c7d7d68..cc413170 100644 --- a/src/morphobricks_preprocessing/run.py +++ b/src/morphobricks_preprocessing/run.py @@ -85,6 +85,7 @@ from .zarr_export import ( ConversionSettings, OmeZarrWriter, + copy_channels_current, copy_units_current, is_copy_path, ome_zarr_relative_path, @@ -515,7 +516,12 @@ def stop(**counts) -> RunResult: significant_bits=header.significant_bits, ome_zarr_path=copy, read_in_blocks=rel in in_blocks, + channel_names_trimmed=header.channel_names_trimmed, ) + if header.channel_names_trimmed: + logger.info( + "Leading or trailing whitespace was removed from the channel names of %s", rel + ) before = previous.file(rel) if previous is not None else None if before is None: new.append(rel) @@ -532,6 +538,7 @@ def stop(**counts) -> RunResult: (output / copy).is_dir() and same_conversion and copy_units_current(output / copy, header.physical_sizes) + and copy_channels_current(output / copy, header.channel_names) ) ) ): diff --git a/src/morphobricks_preprocessing/zarr_export.py b/src/morphobricks_preprocessing/zarr_export.py index 933022b6..d002e98d 100644 --- a/src/morphobricks_preprocessing/zarr_export.py +++ b/src/morphobricks_preprocessing/zarr_export.py @@ -475,18 +475,26 @@ def _omero(self) -> dict: channels = [] for c in range(self.shape[self.roles.index("C")]): low, high = self._channel_ranges.get(c, (0.0, 1.0)) - label = self.channel_names[c] if c < len(self.channel_names) else None - channels.append( - { - "label": label or f"Channel {c}", - "active": True, - "color": "FFFFFF", - "window": {"min": low, "max": high, "start": low, "end": high}, - } - ) + channel = { + "active": True, + "color": "FFFFFF", + "window": {"min": low, "max": high, "start": low, "end": high}, + } + # Only names from the source's metadata are written. An unnamed channel gets + # no label rather than a made-up one, so the copy reads back unnamed, like its + # source, and never fails the exact channel-name check (issue #37). + label = _channel_label(self.channel_names, c) + if label is not None: + channel["label"] = label + channels.append(channel) return {"version": NGFF_VERSION, "channels": channels} +def _channel_label(names: Sequence[str | None], channel: int) -> str | None: + """Return the name of ``channel`` in ``names``; None when it is unnamed.""" + return (names[channel] if channel < len(names) else None) or None + + def is_copy_path(output_folder: str | os.PathLike, relative_path: str) -> bool: """Return True if ``relative_path`` names an OME-Zarr copy inside ``/ome_zarr``. @@ -523,6 +531,35 @@ def copy_units_current(copy: str | os.PathLike, sizes: PhysicalSizes) -> bool: return False +def copy_channels_current(copy: str | os.PathLike, channel_names: Sequence[str | None]) -> bool: + """Return True if a copy's channel labels are exactly the names its source gives now. + + Copies written before unnamed channels were left without a label carry the + placeholder ``Channel ``; such copies are written again on the next run, so the + copy never names a channel its source does not. Only the copy's metadata file is + read. A copy whose metadata cannot be read counts as out of date. + + Parameters + ---------- + copy + The copy's folder (``/ome_zarr/....ome.zarr``). + channel_names + The source image's channel names, as its header gives them (None when unnamed). + """ + try: + attrs = json.loads((Path(copy) / ".zattrs").read_text(encoding="utf-8")) + if "omero" not in attrs: + return True + channels = attrs["omero"]["channels"] + return all( + channel.get("label") == _channel_label(channel_names, c) + for c, channel in enumerate(channels) + ) + except (OSError, ValueError, KeyError, IndexError, TypeError, AttributeError): + logger.debug("Cannot read the channel labels of the copy %s", copy, exc_info=True) + return False + + # zarr 2 / 3 --------------------------------------------------------------------------- diff --git a/src/morphobricks_preprocessing/zarr_input.py b/src/morphobricks_preprocessing/zarr_input.py index 0abec6c6..2e9e13b3 100644 --- a/src/morphobricks_preprocessing/zarr_input.py +++ b/src/morphobricks_preprocessing/zarr_input.py @@ -31,6 +31,7 @@ import logging import os +from collections.abc import Iterable from dataclasses import dataclass from pathlib import Path from typing import Any @@ -52,6 +53,27 @@ class NgffError(ValueError): """Raised when a folder is not an OME-Zarr image this brick can read.""" +def read_channel_names(raw: Iterable[Any]) -> tuple[tuple[str | None, ...], bool]: + """Return channel names as read from metadata, and whether any was trimmed. + + Leading and trailing whitespace is removed from every name, for every image (TIFF, + OME-Zarr, and this brick's own copies), so ``"DAPI "`` and ``"DAPI"`` are one name. + Whitespace inside a name is kept: names are otherwise compared exactly. A name that + is empty, only whitespace or not text is unnamed (None). The names are empty when + none is given. The flag is recorded in the run manifest (``channel_names_trimmed``). + """ + names: list[str | None] = [] + trimmed = False + for value in raw: + if not isinstance(value, str): + names.append(None) + continue + name = value.strip() + trimmed = trimmed or name != value + names.append(name or None) + return (tuple(names) if any(names) else ()), trimmed + + def is_zarr_name(name: str) -> bool: """Return True if a folder name marks a Zarr store (``.zarr``, any case).""" return name.lower().endswith(ZARR_SUFFIX) @@ -97,7 +119,10 @@ class NgffImage: image_count Number of images in the store; more than one blocks processing. channel_names - Channel labels from the ``omero`` metadata; empty when not given. + Channel labels from the ``omero`` metadata, without leading or trailing + whitespace (:func:`read_channel_names`); empty when not given. + channel_names_trimmed + True when leading or trailing whitespace was removed from a label. dataset_path Path of the level-0 array inside the store, e.g. ``"0"`` or ``"0/0"``. physical_sizes @@ -114,6 +139,7 @@ class NgffImage: image_count: int channel_names: tuple[str | None, ...] = () dataset_path: str = "" + channel_names_trimmed: bool = False physical_sizes: PhysicalSizes = PhysicalSizes() @property @@ -274,16 +300,23 @@ def _level0(group, meta: dict, prefix: str, image_count: int) -> NgffImage: if len(axes) != len(array.shape): raise NgffError(f"its metadata names {len(axes)} axes but the array has {len(array.shape)}") omero = meta.get("omero") if isinstance(meta.get("omero"), dict) else {} - labels = tuple( - (channel.get("label") or None) if isinstance(channel, dict) else None + raw_labels = [ + channel.get("label") if isinstance(channel, dict) else None for channel in omero.get("channels") or [] - ) + ] + for index, label in enumerate(raw_labels): + # A label that is not text is malformed metadata: refuse rather than read the + # channel as unnamed, which would hide it from the channel-name check (#37). + if label is not None and not isinstance(label, str): + raise NgffError(f"the label of channel {index} in its omero metadata is not text") + labels, trimmed = read_channel_names(raw_labels) return NgffImage( array=array, axes=axes, version=str(meta.get("version") or first.get("version") or ""), image_count=image_count, - channel_names=labels if any(labels) else (), + channel_names=labels, + channel_names_trimmed=trimmed, dataset_path=f"{prefix}{dataset_path}", physical_sizes=_physical_sizes(first, axes, dataset_path), ) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 1fc5691d..f7086d98 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -175,7 +175,7 @@ def test_unnamed_ome_channels_are_empty(tmp_path): assert info.channel_names == () -def test_differing_channel_names_are_a_warning(): +def test_differing_channel_names_are_an_error(): base = meta("a.tif", (2, 8, 8), axes="CYX") files = [ replace(base, channel_names=("DAPI", "GFP")), @@ -184,11 +184,29 @@ def test_differing_channel_names_are_a_warning(): replace(base, relative_path="d.tif", channel_names=(None, None)), ] found = issue(check_metadata(files), "channel_names_differ") - assert found.severity is Severity.WARNING + assert found.severity is Severity.ERROR # issue #37: channels are matched by number assert found.paths == ("c.tif",) assert "channel 1: GFP (2 images), RFP (1 image)" in found.message +def test_channel_names_differing_only_in_inner_whitespace_are_named_as_such(): + # Inner whitespace is compared exactly (owner decision on #42); the message quotes the + # names and says the difference is whitespace, which would otherwise be invisible. + base = meta("a.tif", (2, 8, 8), axes="CYX") + files = [ + replace(base, channel_names=("DAPI", "GFP 1")), + replace(base, relative_path="b.tif", channel_names=("DAPI", "GFP 1")), + replace(base, relative_path="c.tif", channel_names=("DAPI", "GFP 1")), + ] + found = issue(check_metadata(files), "channel_names_differ") + assert found.severity is Severity.ERROR + assert found.paths == ("c.tif",) + assert ( + 'channel 1: "GFP 1" (2 images), "GFP 1" (1 image) ' + "(the names differ only in spaces or other whitespace)" + ) in found.message + + def test_matching_or_missing_channel_names_give_no_warning(): unnamed = meta("b.tif", (2, 8, 8), axes="CYX") named = replace(unnamed, relative_path="a.tif", channel_names=("A",)) diff --git a/tests/test_run.py b/tests/test_run.py index 664906b7..d112e656 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -220,7 +220,7 @@ def test_manifest_records_labels_mapping_settings_and_files(experiment): def test_manifest_records_channel_names_and_significant_bits(tmp_path): root = tmp_path / "exp" root.mkdir() - for i, names in enumerate([["DAPI", "GFP"], ["DAPI", "RFP"]]): + for i, names in enumerate([["DAPI", "GFP"], ["DAPI", "GFP"]]): tifffile.imwrite( root / f"img_{i}.ome.tif", stack(i, n_z=2), @@ -230,13 +230,69 @@ def test_manifest_records_channel_names_and_significant_bits(tmp_path): ) result = run_experiment(root, prefill_mapping("CYX")) assert result.written - found = issue(result.issues, "channel_names_differ") - assert found.severity is Severity.WARNING + assert "channel_names_differ" not in codes(result.issues) manifest = read_manifest(result.manifest_path) - assert manifest.file("img_1.ome.tif").channel_names == ("DAPI", "RFP") + assert manifest.file("img_1.ome.tif").channel_names == ("DAPI", "GFP") assert manifest.file("img_0.ome.tif").significant_bits == 12 +def write_named(root, names_per_image): + root.mkdir() + for i, names in enumerate(names_per_image): + tifffile.imwrite( + root / f"img_{i}.ome.tif", + stack(i, n_z=2), + ome=True, + photometric="minisblack", + metadata={"axes": "CYX", "Channel": {"Name": names}}, + ) + + +def test_leading_and_trailing_whitespace_of_channel_names_is_removed(tmp_path): + # Owner decision on #42: names are trimmed at read time, for every image, and the + # manifest records that it was done; "DAPI " and "DAPI" are then one name. + root = tmp_path / "exp" + write_named(root, [["DAPI", "GFP"], ["DAPI ", " GFP"]]) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=tmp_path / "out") + assert result.written + assert "channel_names_differ" not in codes(result.issues) + manifest = read_manifest(result.manifest_path) + assert manifest.file("img_1.ome.tif").channel_names == ("DAPI", "GFP") + assert manifest.file("img_1.ome.tif").channel_names_trimmed + assert not manifest.file("img_0.ome.tif").channel_names_trimmed + raw = json.loads(result.manifest_path.read_text(encoding="utf-8")) + assert [f["channel_names_trimmed"] for f in raw["files"]] == [False, True] + + +def test_inner_whitespace_of_channel_names_still_differs(tmp_path): + root = tmp_path / "exp" + write_named(root, [["DAPI", "GFP"], ["DA PI", "GFP"]]) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=tmp_path / "out") + assert not result.written + found = issue(result.issues, "channel_names_differ") + assert found.severity is Severity.ERROR + assert "channel 0: DA PI (1 image), DAPI (1 image)" in found.message + + +def test_differing_channel_names_stop_the_run(tmp_path): + root = tmp_path / "exp" + root.mkdir() + for i, names in enumerate([["DAPI", "GFP"], ["Actin", "GFP"]]): + tifffile.imwrite( + root / f"img_{i}.ome.tif", + stack(i, n_z=2), + ome=True, + photometric="minisblack", + metadata={"axes": "CYX", "Channel": {"Name": names}}, + ) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=tmp_path / "out") + assert not result.written + found = issue(result.issues, "channel_names_differ") + assert found.severity is Severity.ERROR + assert "channel 0: Actin (1 image), DAPI (1 image)" in found.message + assert not (tmp_path / "out" / "plane_histograms.csv").exists() + + # Positions (cross-brick decision D02) ------------------------------------------------- diff --git a/tests/test_zarr_export.py b/tests/test_zarr_export.py index 3dfa1e81..a239949e 100644 --- a/tests/test_zarr_export.py +++ b/tests/test_zarr_export.py @@ -11,7 +11,9 @@ AxisMapping, PhysicalSizes, PlaneReadError, + check_metadata, default_output_folder, + inspect_ome_zarr, ome_zarr_relative_path, plane_layout, prefill_mapping, @@ -384,6 +386,51 @@ def test_channel_names_and_display_ranges(tmp_path): assert (window["min"], window["max"]) == (float(data[1].min()), float(data[1].max())) +@needs_zarr +def test_copies_of_unnamed_channels_read_back_unnamed(tmp_path): + # Issue #37 review: channel names are compared exactly, so a copy must not name a + # channel its source leaves unnamed (it used to write the placeholder "Channel "). + root = tmp_path / "exp" + root.mkdir() + tifffile.imwrite( + root / "a.ome.tif", + random_data((2, 8, 8)), + ome=True, + photometric="minisblack", + metadata={"axes": "CYX", "Channel": {"Name": ["DAPI", "GFP"]}}, + ) + write_data(root / "b.tif", random_data((2, 8, 8), seed=1), axes="CYX") + result = run_experiment(root, prefill_mapping("CYX"), convert_tiffs=True) + assert result.written + omero = dict(open_copy(result.output_folder, "b.tif").attrs)["omero"] + assert all("label" not in channel for channel in omero["channels"]) + copies = result.output_folder / "ome_zarr" + headers = [inspect_ome_zarr(copies / name, name) for name in ("a.ome.zarr", "b.ome.zarr")] + assert [h.channel_names for h in headers] == [("DAPI", "GFP"), ()] + assert "channel_names_differ" not in {i.code for i in check_metadata(headers)} + + +@needs_zarr +def test_copies_with_placeholder_channel_names_are_written_again(tmp_path): + root = tmp_path / "exp" + root.mkdir() + write_data(root / "a.tif", random_data((2, 8, 8)), axes="CYX") + first = run_experiment(root, prefill_mapping("CYX"), convert_tiffs=True) + table = first.csv_path.read_bytes() + # Make the copy look like one written with placeholder names. + attrs_path = first.output_folder / ome_zarr_relative_path("a.tif") / ".zattrs" + attrs = json.loads(attrs_path.read_text(encoding="utf-8")) + for c, channel in enumerate(attrs["omero"]["channels"]): + channel["label"] = f"Channel {c}" + attrs_path.write_text(json.dumps(attrs), encoding="utf-8") + second = run_experiment(root, prefill_mapping("CYX"), convert_tiffs=True) + assert second.converted == ("a.tif",) + assert second.csv_path.read_bytes() == table # same pixels, same rows + omero = dict(open_copy(second.output_folder, "a.tif").attrs)["omero"] + assert all("label" not in channel for channel in omero["channels"]) + assert run_experiment(root, prefill_mapping("CYX"), convert_tiffs=True).converted == () + + @needs_zarr def test_missing_axis_becomes_length_one(tmp_path): write_data(tmp_path / "exp/a.tif", random_data((2, 3, 8, 8)), axes="ZTYX") diff --git a/tests/test_zarr_input.py b/tests/test_zarr_input.py index 6f1048c3..a0406bc0 100644 --- a/tests/test_zarr_input.py +++ b/tests/test_zarr_input.py @@ -112,6 +112,42 @@ def test_open_ngff(tmp_path, version): assert image.chunks == (1, 1, 8, 9) +@pytest.mark.parametrize("version", VERSIONS) +def test_channel_labels_are_trimmed_but_inner_whitespace_kept(tmp_path, version): + # Owner decision on #42: the same trimming as for TIFF names, for every image. + path = write_ngff( + tmp_path / "a.ome.zarr", + random_data((3, 8, 9)), + "cyx", + version=version, + channel_names=[" DAPI ", "GFP 1", " "], + ) + image = open_ngff(path) + assert image.channel_names == ("DAPI", "GFP 1", None) + assert image.channel_names_trimmed + plain = write_ngff( + tmp_path / "b.ome.zarr", + random_data((1, 8, 9)), + "cyx", + version=version, + channel_names=["DAPI"], + ) + assert not open_ngff(plain).channel_names_trimmed + + +@pytest.mark.parametrize("label", [1, ["DAPI"], {"name": "DAPI"}, True]) +def test_channel_label_that_is_not_text_is_refused(tmp_path, label): + # Review of #42: a non-text label was read as an unnamed channel, silently skipping + # the channel-name check; it is malformed metadata and refused instead. + path = write_ngff( + tmp_path / "a.ome.zarr", random_data((2, 8, 9)), "cyx", channel_names=["DAPI", label] + ) + with pytest.raises(NgffError, match="channel 1"): + open_ngff(path) + header = inspect_image(path, "a.ome.zarr") + assert not header.readable + + # Physical sizes (OME-NGFF coordinateTransformations and axis units) -------------------- MICRONS = {"z": "micrometer", "y": "micrometer", "x": "micrometer"}