diff --git a/CHANGELOG.md b/CHANGELOG.md index ca2f3c88..dd6299b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ are in [docs/plan.md](docs/plan.md) ยง12. Upgrading from v0.3: see the ## Unreleased +- **Described runs with several layouts report `R_HETEROGENEOUS_LAYOUT_NOT_ENABLED` first (#245).** + The run lists it before the axis review's issues, and the arrangement issues + (`inconsistent_mapping`, `missing_axes`, `ambiguous_missing_axes`) no longer advise exporting the + images the same way: they stay errors and say they follow from the layouts. The notebook's axis + step says that no roles are suggested because the units declare different layouts (fluorescence + and RGB). The refusal codes are unchanged: nothing refused before is accepted. - **Notebook: one step at a time (UX proposal 2026-11-09, PR 17a).** `PreprocessingNotebook.steps()` shows the shipped notebook's 13 steps one at a time, with a strip of chips (done green with a tick, current blue, to do grey, changed by a reopen orange; a chip opens its step) and Back/Next (Next is diff --git a/src/morphobricks_preprocessing/description_layouts.py b/src/morphobricks_preprocessing/description_layouts.py index 014ea569..4ddbcbdf 100644 --- a/src/morphobricks_preprocessing/description_layouts.py +++ b/src/morphobricks_preprocessing/description_layouts.py @@ -88,3 +88,11 @@ def heterogeneous_units(layouts: tuple[DeclaredLayout, ...]) -> tuple[str, ...]: return tuple( f"{_label(index)}: {unit}" for index, layout in enumerate(layouts) for unit in layout.units ) + + +def layout_kinds(layouts: tuple[DeclaredLayout, ...]) -> str: + """Name the kinds of the layouts in words, e.g. ``"fluorescence and RGB"``.""" + kinds = {"RGB" if layout.image_type == "rgb" else "fluorescence" for layout in layouts} + if len(kinds) == 1: + return f"{kinds.pop()}, with different channel lists" + return " and ".join(sorted(kinds, key=lambda kind: kind == "RGB")) diff --git a/src/morphobricks_preprocessing/notebook_axis_step.py b/src/morphobricks_preprocessing/notebook_axis_step.py index a5328478..8f4097bb 100644 --- a/src/morphobricks_preprocessing/notebook_axis_step.py +++ b/src/morphobricks_preprocessing/notebook_axis_step.py @@ -10,6 +10,7 @@ from .axes import axis_length_ranges, axis_name, file_mapping_for from .axis_check import AXIS_CHECK_CODES from .axis_review import review_axes +from .description_layouts import declared_layouts, is_heterogeneous, layout_kinds from .image_type import FLUORESCENCE from .notebook_axes import axis_check_html, header_marks from .notebook_options import IMAGE_TYPE_OPTIONS, ROLE_OPTIONS @@ -194,6 +195,18 @@ def _axis_waiting(self) -> str: if self.preflight is None: return ui.status("info", "Waiting for step 1.", ["Check the experiment folder first."]) if self.mapping is None: + kinds = self._layout_kinds() + if kinds: + return ui.status( + "error", + "No axis roles are suggested: the units declare different layouts.", + [ + f"The saved description declares different layouts ({kinds}). " + "This version runs one layout per experiment, so the " + "run stops with R_HETEROGENEOUS_LAYOUT_NOT_ENABLED (see Check and " + "save). Exporting the images again would not change that." + ], + ) return ui.status( "error", "No axis roles can be suggested.", @@ -205,6 +218,14 @@ def _axis_waiting(self) -> str: ) return "" + def _layout_kinds(self) -> str: + """Return the kinds of layout the saved description declares when they differ, else ''.""" + state = self._published() if getattr(self, "_describe", False) else None + if state is None: + return "" + layouts = declared_layouts(state.description) + return layout_kinds(layouts) if is_heterogeneous(layouts) else "" + def _role_grid(self) -> widgets.GridBox: """Build one box per axis, with its length, the suggested role and a dropdown.""" ranges = axis_length_ranges(self.mapping, self.preflight.metadata.files) diff --git a/src/morphobricks_preprocessing/run/description_run.py b/src/morphobricks_preprocessing/run/description_run.py index 5705730b..23708c11 100644 --- a/src/morphobricks_preprocessing/run/description_run.py +++ b/src/morphobricks_preprocessing/run/description_run.py @@ -29,7 +29,7 @@ from ..description_record import description_record, unit_records from ..description_run_text import heterogeneous_issue, physical_issue, reader_issue from ..mask_representation import NOT_CHECKED, file_status, read_checked -from ..physical_check import PhysicalReport, check_physical +from ..physical_check import HETEROGENEOUS_LAYOUT, PhysicalReport, check_physical from ..unit_consistency import declared_type_issues from ..validation import ValidationIssue, error from .description_file import RecordsFileError, read_records @@ -163,9 +163,40 @@ def checked_description(output: Path, pre: Preflight, checked: Checked) -> RunDe roles = None if checked.review is None else checked.review.file_mappings found, refused = check_description(found, pre, roles) checked.issues.extend(refused) + checked.issues[:] = heterogeneous_first(checked.issues) return found +SECONDARY_CODES = frozenset({"inconsistent_mapping", "missing_axes", "ambiguous_missing_axes"}) +"""The axis review's arrangement codes, whose advice is to export the images again.""" + +SECONDARY_ADVICE = ( + "Do not export the images again for this: it follows from the different layouts " + f"({HETEROGENEOUS_LAYOUT}, listed first). This version runs one layout per experiment." +) + + +def heterogeneous_first(issues: Iterable[ValidationIssue]) -> list[ValidationIssue]: + """Put the layout refusal first and make the axis review's arrangement issues secondary. + + With several layouts the arrangement of dimensions differs between units by design, so the + axis review's arrangement issues (:data:`SECONDARY_CODES`) are a consequence, and + exporting the images the same way would not help: their advice is replaced. Only the order + and the advice change: no issue is added, removed or turned into a warning, so the run + refuses exactly as before. + """ + found = list(issues) + layout = [i for i in found if i.code == HETEROGENEOUS_LAYOUT] + if not layout: + return found + rest = [ + replace(i, action=SECONDARY_ADVICE) if i.code in SECONDARY_CODES else i + for i in found + if i.code != HETEROGENEOUS_LAYOUT + ] + return [*layout, *rest] + + def mask_records( description: Description, masks: Mapping | None, output: Path, images: int ) -> dict[str, dict]: diff --git a/tests/test_heterogeneous_refusal_first.py b/tests/test_heterogeneous_refusal_first.py new file mode 100644 index 00000000..0a5fe281 --- /dev/null +++ b/tests/test_heterogeneous_refusal_first.py @@ -0,0 +1,136 @@ +"""With several layouts the layout refusal comes first and the axis advice follows (issue #245). + +The Spec refuses an RGB unit next to a fluorescence unit (``R_HETEROGENEOUS_LAYOUT_NOT_ENABLED``, +fixture v29). The axis review's ``inconsistent_mapping`` is a consequence of the layouts, so its +advice to export the images the same way must not lead. Nothing refused before is accepted: the +codes of the refusals are exactly those of before. +""" + +from itertools import combinations + +import pytest + +from description_tree import build_experiment +from morphobricks_preprocessing import prefill_mapping, preflight, run_experiment +from morphobricks_preprocessing.axis_review import review_axes +from morphobricks_preprocessing.description import description_from_bytes +from morphobricks_preprocessing.description_candidate import check_candidate +from morphobricks_preprocessing.image_consistency import arrangement_issues +from morphobricks_preprocessing.notebook_check_view import checks_html +from morphobricks_preprocessing.unit_consistency import unit_checked +from test_described_unit_checks import ( + LAYOUT, + VALID, + _checked_notebook, + mixed_experiment, + publish, +) +from test_metadata import meta + + +def test_the_layout_refusal_comes_first_in_the_run(tmp_path): + root, out = mixed_experiment(tmp_path) + publish(out) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=out) + assert not result.written + assert [i.code for i in result.issues][0] == LAYOUT + assert {i.code for i in result.issues} == {LAYOUT, "inconsistent_mapping"} + + +def test_the_axis_issue_stays_an_error_without_the_advice_to_export_again(tmp_path): + root, out = mixed_experiment(tmp_path) + publish(out) + result = run_experiment(root, prefill_mapping("CYX"), output_folder=out) + (axis,) = [i for i in result.issues if i.code == "inconsistent_mapping"] + assert axis.is_error + assert "Export the images again" not in axis.action + assert LAYOUT in axis.action and "one layout per experiment" in axis.action + + +def test_a_run_of_one_layout_keeps_the_advice_of_the_axis_review(tmp_path): + description = description_from_bytes(VALID["v01"].data) + root, out = tmp_path / "exp", tmp_path / "out" + out.mkdir() + (out / "morphobricks.toml").write_bytes(VALID["v01"].data) + build_experiment(root, description, images=2) + result = run_experiment(root, prefill_mapping("YXC"), output_folder=out) + (axis,) = [i for i in result.issues if i.code == "inconsistent_mapping"] + assert "Export the images again" in axis.action + assert LAYOUT not in {i.code for i in result.issues} + + +@pytest.mark.parametrize("confirmed", [False, True]) +def test_the_check_and_save_list_leads_with_the_layout_refusal(tmp_path, confirmed): + root, out = mixed_experiment(tmp_path) + pre = unit_checked(preflight(root, out)) + roles = review_axes(pre.metadata, prefill_mapping("CYX")).file_mappings if confirmed else None + check = check_candidate(description_from_bytes(VALID["v29"].data), pre, roles, out) + assert check.run_refusals[0].code == LAYOUT + assert check.heterogeneous is check.run_refusals[0] + text = checks_html(check) + assert "2 channel layouts" in text + assert "inconsistent_mapping" not in text + + +def test_the_axis_step_says_the_layouts_are_why_no_roles_are_suggested(tmp_path): + root, out = mixed_experiment(tmp_path) + publish(out) + nb = _checked_notebook(root, out, describe=True) + assert nb.mapping is None + text = nb._axis_waiting() + assert "the units declare different layouts" in text + assert "fluorescence and RGB" in text + assert "one layout per experiment" in text + assert "dimensions are arranged differently" not in text + + +def test_the_axis_step_keeps_its_general_message_without_a_description(tmp_path): + root, out = mixed_experiment(tmp_path) + nb = _checked_notebook(root, out, describe=True) + assert nb.mapping is None + text = nb._axis_waiting() + assert "No axis roles can be suggested." in text + assert "different layouts" not in text + + +BASE = ["TCZYX", "TCYX", "CTYX", "TZYX", "CYX", "TYX", "ZYX", "YX"] + + +def _dropped(short, long): + rest = iter(long) + return all(axis in rest for axis in short) + + +def _found(*axes): + files = [meta(f"{n}.tif", axes=a) for n, a in enumerate(axes)] + return {i.code for i in arrangement_issues(files)} + + +def test_missing_axes_means_one_longest_arrangement_and_the_rest_with_axes_dropped(): + """Pin the ``missing_axes`` rule against an independent statement of it (issue #243 review). + + The clause ``sum(len(a) == len(longest) ...) == 1`` is equivalent to ``>= 1``: two different + arrangements of the longest length are never both subsequences of it, so the ``all(...)`` + before it already refuses them. Mutating it to ``>= 1`` therefore changes no behaviour and no + test can kill it. What this test fixes is the behaviour, for every pair and triple of + arrangements: ``missing_axes`` only when every image is the longest with axes dropped, else + ``inconsistent_dimensions``, and nothing for one arrangement. + """ + for group in [*combinations(BASE, 2), *combinations(BASE, 3)]: + longest = max(group, key=len) + dropped = all(_dropped(axes, longest) for axes in group) + assert _found(*group) == {"missing_axes" if dropped else "inconsistent_dimensions"}, group + assert _found("CYX", "CYX") == set() + + +def test_layout_kinds_names_two_channel_lists_of_one_image_type_in_words(): + """Two fluorescence layouts differ by channels, not by type: say so, not "fluorescence".""" + from morphobricks_preprocessing.description_layouts import DeclaredLayout, layout_kinds + + both = ( + DeclaredLayout(("DNA",), "fluorescence", ()), + DeclaredLayout(("DNA", "ACTIN"), None, ()), + ) + assert layout_kinds(both) == "fluorescence, with different channel lists" + mixed = (both[0], DeclaredLayout(("R", "G", "B"), "rgb", ())) + assert layout_kinds(mixed) == "fluorescence and RGB"