From 407395c77342505de9b62a345623cbc863b93c8f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 03:59:09 +0000 Subject: [PATCH] Description reader owns its rule vocabulary; pin five refusals by path (#204) description_identity no longer imports check_identity_text from discovery or SINGLE from normalization (Spec 2.7): description_rules holds MISSING_TEXT, SINGLE and label_refused, and a test keeps them equal to the shared values. Direct reader tests pin the path and value of E_SOURCE_FOLDER_REUSED, E_SOURCE_FOLDER_CONFLICT, E_SOURCE_REPEAT_REUSED, E_MASK_SOURCE_TYPE_REUSED and E_REFERENCE_CYCLE. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Scma6W3LC11s3qjXHZuGDm --- CHANGELOG.md | 8 ++ .../description_identity.py | 11 +-- .../description_rules.py | 17 ++++ tests/spec_gate.py | 1 + tests/test_description_rules_copy.py | 38 ++++++++ tests/test_spec_reader_refusal_paths.py | 93 +++++++++++++++++++ tests/test_spec_rules_table.py | 7 +- 7 files changed, 163 insertions(+), 12 deletions(-) create mode 100644 tests/test_description_rules_copy.py create mode 100644 tests/test_spec_reader_refusal_paths.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ecb35d77..e1a87d45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ are in [docs/plan.md](docs/plan.md) §12. Upgrading from v0.3: see the ## Unreleased +- **Reader owns its rule vocabulary; five refusals pinned by path (#204).** + `description_identity` no longer imports `check_identity_text` from `discovery` or `SINGLE` + from normalization (Spec §2.7): `description_rules` holds `MISSING_TEXT`, `SINGLE` and + `label_refused`, and `tests/test_description_rules_copy.py` keeps them equal to the shared + values. Behaviour is unchanged. `tests/test_spec_reader_refusal_paths.py` pins the path and + value of `E_SOURCE_FOLDER_REUSED`, `E_SOURCE_FOLDER_CONFLICT`, `E_SOURCE_REPEAT_REUSED`, + `E_MASK_SOURCE_TYPE_REUSED` and `E_REFERENCE_CYCLE`, which the oracle compares by code only. + The reader's paths were already correct for all five. - **Semantic `RunInputs` (Stage 1, PR 12; stacked on PR 11).** For a run with a published description, each `Fov` carries `repeat_group`, `condition_id`, `technical_repeat_id` and `channel_ids` beside the unchanged key (owner decision Q8: the key keeps the source folder diff --git a/src/morphobricks_preprocessing/description_identity.py b/src/morphobricks_preprocessing/description_identity.py index 23156a53..ac5841fc 100644 --- a/src/morphobricks_preprocessing/description_identity.py +++ b/src/morphobricks_preprocessing/description_identity.py @@ -1,6 +1,6 @@ """Validation phase 4, identity (experiment-v1 sections 3 and 6). -The Label rules (the brick's ``check_identity_text``, decision D-e), the Token rule, the reserved +The Label rules (the reader's own ``label_refused``, decision D-e), the Token rule, the reserved list, NFC, ``single`` and the case-collision scopes. Each broken rule has its own code and every rule an identity breaks is reported (one code per broken rule). Nothing is trimmed, normalized or repaired: the NFC form of a value is a comparison key only and never leaves this module. @@ -11,12 +11,9 @@ import unicodedata from collections.abc import Mapping -from morphobricks_normalization.records import SINGLE - from .description_report import Issues, key_path -from .description_rules import OBJECT_SET_PART, RESERVED, TOKEN, Code +from .description_rules import OBJECT_SET_PART, RESERVED, SINGLE, TOKEN, Code, label_refused from .description_walk import Table, member_path, members, unit_path -from .discovery import IdentityError, check_identity_text USED_KINDS = ("conditions", "channels", "repeats", "technical_repeats") @@ -35,9 +32,7 @@ def __init__(self, out: Issues) -> None: def label(self, value: str, path: str) -> None: """Apply the Label rules (``E_ID_TEXT``).""" - try: - check_identity_text(value, "identity") - except IdentityError: + if label_refused(value): self.out.add(Code.E_ID_TEXT, path, value) def token(self, value: str, path: str) -> None: diff --git a/src/morphobricks_preprocessing/description_rules.py b/src/morphobricks_preprocessing/description_rules.py index 96f28dd2..36cae9ff 100644 --- a/src/morphobricks_preprocessing/description_rules.py +++ b/src/morphobricks_preprocessing/description_rules.py @@ -162,3 +162,20 @@ class TableSpec: } ) # fmt: skip """The one reserved list of channel IDs and mask IDs, compared case-folded (section 3.2).""" + +MISSING_TEXT = frozenset({"", "na", "nan", "none", "null", "n/a", ""}) +"""The names the Label rule refuses, compared case-folded: table readers read them as a missing +value (section 3.2). The reader's own copy; ``tests/test_description_rules_copy.py`` keeps it +equal to the value discovery uses.""" +SINGLE = "single" +"""The one lower-case label of a level with a single member (section 3.3). The reader's own +copy; a test keeps it equal to the value normalization uses.""" + + +def label_refused(value: str) -> bool: + """Return whether the Label rule (section 3.2) refuses a text value. + + A label is refused when it starts or ends with whitespace or, case-folded, is one of + :data:`MISSING_TEXT`. Nothing is trimmed or repaired. + """ + return value != value.strip() or value.casefold() in MISSING_TEXT diff --git a/tests/spec_gate.py b/tests/spec_gate.py index 045a5772..e8498418 100644 --- a/tests/spec_gate.py +++ b/tests/spec_gate.py @@ -37,6 +37,7 @@ "test_spec_publication.py", "test_spec_reader.py", "test_spec_reader_phases_1_3.py", + "test_spec_reader_refusal_paths.py", "test_spec_reconcile.py", "test_spec_refval_crosscheck.py", "test_spec_rules_table.py", diff --git a/tests/test_description_rules_copy.py b/tests/test_description_rules_copy.py new file mode 100644 index 00000000..7f8358eb --- /dev/null +++ b/tests/test_description_rules_copy.py @@ -0,0 +1,38 @@ +"""The reader's own copy of two rules equals the values discovery and normalization use. + +Spec section 2.7: the reader takes no vocabulary from outside itself at run time, so +``description_rules`` holds its own ``MISSING_TEXT`` and ``SINGLE``. These tests keep them equal +to the shared values, so a later change to either is caught here. +""" + +from __future__ import annotations + +import pytest +from morphobricks_normalization.records import SINGLE as NORMALIZATION_SINGLE + +from morphobricks_preprocessing import description_rules as rules +from morphobricks_preprocessing.discovery import ( + MISSING_TEXT as DISCOVERY_MISSING_TEXT, +) +from morphobricks_preprocessing.discovery import IdentityError, check_identity_text + + +def test_the_reader_s_missing_text_equals_discoverys(): + assert rules.MISSING_TEXT == DISCOVERY_MISSING_TEXT + + +def test_the_reader_s_single_equals_normalizations(): + assert rules.SINGLE == NORMALIZATION_SINGLE + + +@pytest.mark.parametrize( + "value", + ["", "NA", "Nan", "NONE", "null", "N/A", "", " a", "a ", "\ta", "ok", "single", "x y"], +) +def test_the_label_rule_agrees_with_check_identity_text(value): + try: + check_identity_text(value, "identity") + shared_refuses = False + except IdentityError: + shared_refuses = True + assert rules.label_refused(value) is shared_refuses diff --git a/tests/test_spec_reader_refusal_paths.py b/tests/test_spec_reader_refusal_paths.py new file mode 100644 index 00000000..7deef513 --- /dev/null +++ b/tests/test_spec_reader_refusal_paths.py @@ -0,0 +1,93 @@ +"""The reader names the offending field and value of five codes the oracle compares only by code. + +``tests/spec_oracle.py`` (``COARSE``) drops the paths of these codes, because the oracle names +a table and the reader names each field. A refusal attached to the wrong field would therefore +pass the crosscheck; these tests pin the path and the value of each, on the Spec fixtures that +trigger them. +""" + +from __future__ import annotations + +import pytest + +from morphobricks_preprocessing.description_read import read_description_bytes +from spec_oracle import COARSE +from spec_pin import fixtures + +BY_NAME = {item.name: item for item in fixtures("invalid")} +UNIT = "repeats.rep1.members.{}.technical_repeats.acq1.source_folder" + +CASES = { + "i122_two_condition_ids_one_folder": ( + "E_SOURCE_FOLDER_REUSED", + { + ("conditions.CTRL.source_folder", "control"), + ("conditions.DrugA.source_folder", "control"), + }, + ), + "i123_two_technical_repeat_ids_one_folder": ( + "E_SOURCE_FOLDER_REUSED", + { + ( + "repeats.rep1.members.CTRL.technical_repeats.acq1.source_folder", + "acquisition_1", + ), + ( + "repeats.rep1.members.CTRL.technical_repeats.acq2.source_folder", + "acquisition_1", + ), + }, + ), + "i124_one_technical_repeat_id_two_folders": ( + "E_SOURCE_FOLDER_CONFLICT", + { + (UNIT.format("CTRL"), "acquisition_1"), + (UNIT.format("DrugA"), "acq_1"), + }, + ), + "i74_source_folder_in_two_repeats": ( + "E_SOURCE_REPEAT_REUSED", + { + ("repeats.Day2.members.CTRL.source_repeat", "exp7"), + ("repeats.Day3.members.CTRL.source_repeat", "exp7"), + }, + ), + "i80_mask_source_type_reused": ( + "E_MASK_SOURCE_TYPE_REUSED", + { + ("masks.Nuclei.source_type", "nuclei"), + ("masks.Nuclei2.source_type", "nuclei"), + }, + ), + "i64_reference_cycle": ( + "E_REFERENCE_CYCLE", + { + ("conditions.CTRL.reference", ("CTRL", "DrugA")), + ("conditions.DrugA.reference", ("CTRL", "DrugA")), + }, + ), + "i65_reference_cycle_long": ( + "E_REFERENCE_CYCLE", + { + (f"conditions.{name}.reference", ("CTRL", "DrugA", "DrugB")) + for name in ("CTRL", "DrugA", "DrugB") + }, + ), +} + + +def test_the_cases_cover_the_five_codes_the_oracle_compares_by_code(): + assert {code for code, _ in CASES.values()} == COARSE + + +@pytest.mark.parametrize("name", CASES) +def test_each_refusal_is_attached_to_its_offending_field_with_its_value(name): + code, expected = CASES[name] + issues = read_description_bytes(BY_NAME[name].data).errors + found = { + (issue.path, tuple(issue.value) if isinstance(issue.value, list) else issue.value) + for issue in issues + if issue.code.value == code + } + assert found == expected + assert len([i for i in issues if i.code.value == code]) == len(expected) diff --git a/tests/test_spec_rules_table.py b/tests/test_spec_rules_table.py index ef759d57..36e30211 100644 --- a/tests/test_spec_rules_table.py +++ b/tests/test_spec_rules_table.py @@ -13,7 +13,6 @@ from morphobricks_preprocessing import description_rules as rules from morphobricks_preprocessing.description_rules import Code, TableSpec -from morphobricks_preprocessing.discovery import MISSING_TEXT from spec_text import ( BACKTICKED, section, @@ -90,6 +89,6 @@ def test_the_token_pattern_and_the_rgb_tokens_are_the_text(): def test_the_label_rule_the_reader_calls_refuses_the_names_of_the_text(): - # The Label rule is the brick's check_identity_text (decision D-e), not a copy: its list - # of refused names must be the one section 3.2 states. - assert MISSING_TEXT == spec_label_sentinels() + # The reader's own Label rule (decision D-e): its list of refused names must be the one + # section 3.2 states; test_description_rules_copy keeps it equal to discovery's. + assert rules.MISSING_TEXT == spec_label_sentinels()