diff --git a/PIPELINE_SETTINGS.md b/PIPELINE_SETTINGS.md index 002ec05..0796211 100644 --- a/PIPELINE_SETTINGS.md +++ b/PIPELINE_SETTINGS.md @@ -111,4 +111,5 @@ run measure the default while the operator believes otherwise. | flag | removed | why | |---|---|---| +| `LNG_SHARE_VARIANT_NODES` | 2026-09-21 | Variant-node sharing is always on. Edges -21.5%, nodes -34.6% (no-op in 47 of 92 pathways), no pathway gains cyclic nodes, held-out net zero on both axes (deltasignal `specs/020-variant-node-sharing`). | | `LNG_BOUNDARY_LEAF_REUSE` | 2026-09-21 | Boundary leaves never reuse a node the root complex can reach at the time the leaf is chosen. Held-out +173, p<1e-4 (deltasignal `specs/018-derived-edge-loops`). | diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 622a59a..5015893 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -488,6 +488,9 @@ def _register_entity_uuid( # measures the default. That silent-input failure mode has bitten this project # four times (deltasignal DS #40-43). _REMOVED_ENV = { + "LNG_SHARE_VARIANT_NODES": + "variant-node sharing is always on. See deltasignal " + "specs/020-variant-node-sharing.", "LNG_BOUNDARY_LEAF_REUSE": "boundary leaves never reuse a node the root complex can reach at the time " "the leaf is chosen. See deltasignal specs/018-derived-edge-loops.", @@ -509,7 +512,6 @@ def _register_phase1( terminal_output_eids: Set[str], terminal_output_uuid_cache: Dict[str, str], vr_to_reaction: Optional[Dict[str, str]] = None, - share_variants: bool = False, ) -> Dict[str, int]: """Phase 1 of UUID assignment: one key per (entity, virtual reaction, role). @@ -517,19 +519,40 @@ def _register_phase1( reactions, one per member combination, and every one of them registers its own UUID for every participant. An entity common to all variants -- BCDX2 in HDR's strand-invasion reactions -- therefore exists as one node PER VARIANT: - 33 copies from 5 reactions, each a full copy with the same downstream edges. + 33 copies from 6 reactions, each a full copy with the same downstream edges. Nothing downstream can tell them apart, and any solver rule over the copies is wrong in one direction or the other: min over copies caps the container by whichever copy a perturbation missed (missed change); max lets one - elevated copy lift it (false change). Both were measured (deltasignal - specs/016): -83 and -117 held-out. - - `share_variants` collapses exactly that: the SAME entity in the SAME role - across the variants of ONE Reactome reaction shares one UUID. Variants that - differ in WHICH set member they use have different entity ids and are not - conflated. Copies across DIFFERENT reactions stay positional, as designed - (bridging those was measured harmful four times). Boundary entities already - share per stId through their caches and are left to them. + elevated copy lift it (false change). Both were measured on 81 pathways + (deltasignal specs/016 P12): min over copies **-85 held-out** (p 0.0021), + max over copies **-322** (p<0.0001). (An earlier revision of this docstring + cited "-83 and -117"; -83 was a superseded intermediate and -117 is a + DSB-Repair-only ACCURACY delta, not a held-out figure.) + + The registry KEY is unchanged -- Phase 2 looks entities up by virtual + reaction (`entity_uuid_registry.get((on, p_vr, "output"))`) and would break + if it moved. What changed is the UUID *value*: the keys of the SAME entity + in the SAME role across the variants of ONE Reactome reaction now share one + value, because the variant cache is keyed on the Reactome reaction. + Variants that differ in WHICH set member they use have different entity ids + and are not conflated. Copies across DIFFERENT reactions stay positional, as + designed (bridging those was measured harmful four times). Boundary entities + already share per stId through their caches and are left to them. + + This is unconditional; the LNG_SHARE_VARIANT_NODES flag that gated it was + removed once measured (deltasignal specs/020). Edges fall 21.5% and nodes + 34.6%, but the node figure flatters it: sharing is a no-op in 47 of 92 + pathways and the median reduction among the 45 that move is 6.7%, so the + spec names the edge figure as the conservative read. No pathway gains + cyclic nodes, so this does not undo the specs/018 unwelding. 29 of 24,100 + predictions change; among cases both arms resolve identically the held-out + net is zero on both ground-truth axes, though that conditioning drops 4,094 + cases as non-comparable and unconditioned both axes fall slightly + (curator 20414->20400, experimental 611->609). Note most of the Phase-1 + photocopying is already undone downstream -- Phase 2 unions producer-output + with consumer-input across variant pairs, and the boundary caches share per + stId -- so only 1,431 of 29,655 (entity, reaction, role) contexts actually + carry more than one node, and catalyst/regulator contexts carry none. Returns a small stats dict for the log. """ @@ -539,7 +562,7 @@ def _register_phase1( unmapped = 0 for vr_uid, (input_ids, output_ids, *_) in vr_entities.items(): rxn = vr_to_reaction.get(str(vr_uid)) - if share_variants and rxn in (None, "", "nan", "None"): + if rxn in (None, "", "nan", "None"): # `reactome_id` read back from a cached CSV with no dtype turns a # missing id into the STRING "nan"; treated as a real key it would # collapse every such variant across reactions onto one uuid. @@ -550,7 +573,7 @@ def _register_phase1( ): for eid in ids: key = (eid, vr_uid, role) - if (share_variants and rxn not in (None, "", "nan", "None") + if (rxn not in (None, "", "nan", "None") and eid not in boundary_eids and key not in entity_uuid_registry): vkey = (eid, rxn, role) @@ -564,12 +587,11 @@ def _register_phase1( continue _register_entity_uuid(eid, vr_uid, role, entity_uuid_registry, boundary_eids, boundary_cache) - if share_variants: - logger.info(f"Variant-node sharing: {shared} (entity, reaction, role) registrations " - f"reused a sibling variant's UUID ({len(variant_cache)} distinct)") - if unmapped: - logger.warning(f"Variant-node sharing: {unmapped} virtual reactions have no Reactome " - f"reaction id and were registered per-variant (sharing is inconsistent for them)") + logger.info(f"Variant-node sharing: {shared} (entity, reaction, role) registrations " + f"reused a sibling variant's UUID ({len(variant_cache)} distinct)") + if unmapped: + logger.warning(f"Variant-node sharing: {unmapped} virtual reactions have no Reactome " + f"reaction id and were registered per-variant (sharing is inconsistent for them)") return {"shared": shared, "distinct": len(variant_cache), "unmapped": unmapped} @@ -2174,15 +2196,18 @@ def create_pathway_logic_network( # Each entity gets a unique UUID per (entity, reaction, role) triple. # No cross-role keys are created (unlike the old self-loop approach). # Boundary entities (root inputs / terminal outputs) share one UUID per stId. - # Under LNG_SHARE_VARIANT_NODES the same entity in the same role across the - # VARIANTS of one Reactome reaction shares one UUID (see _register_phase1). + # The same entity in the same role across the VARIANTS of one Reactome + # reaction shares one UUID (see _register_phase1). This was the + # LNG_SHARE_VARIANT_NODES flag; it is now unconditional, and setting the + # variable is an error rather than a no-op so a stale value in a script is + # not silently ignored. + _reject_removed_env() _register_phase1( vr_entities, entity_uuid_registry, root_input_eids, root_input_uuid_cache, terminal_output_eids, terminal_output_uuid_cache, vr_to_reaction=dict(zip(reaction_id_map["uid"].astype(str), reaction_id_map["reactome_id"].astype(str))), - share_variants=os.environ.get("LNG_SHARE_VARIANT_NODES", "0") == "1", ) logger.debug(f"Phase 1 complete: {len(entity_uuid_registry)} registry entries") diff --git a/src/pathway_generator.py b/src/pathway_generator.py index 34f61ca..f7edf92 100755 --- a/src/pathway_generator.py +++ b/src/pathway_generator.py @@ -36,6 +36,18 @@ "LNG_HANDOFF_HUB_MAX", "LNG_SET_MEMBERS_OR", "LNG_DIAGRAM_SET_MEMBER", + # Of these three, only LNG_EMIT_ONE_SIDED is a genuine cache gap: it is + # read in reaction_generator.decompose_by_reactions, so it changes + # best_matches.csv / decomposed_uid_mapping.csv, which ARE cached. + # LNG_BOUNDARY_EXPANSION and LNG_COMPOSITION_EDGES are read inside + # create_pathway_logic_network, which is re-run from scratch every time, so + # listing them buys provenance in fingerprint.json at the cost of an + # unnecessary re-fetch when they are flipped. Kept deliberately: knowing + # which settings produced a catalog has been worth more than the re-fetch, + # and src_sha256 already invalidates on any source change anyway. + "LNG_BOUNDARY_EXPANSION", + "LNG_COMPOSITION_EDGES", + "LNG_EMIT_ONE_SIDED", # Determinism controls: node ids are uuid4 and several selections iterate # sets, so hash seeding changes emitted content (~5.8% of TP53 edges per # bin/create-pathways.py). A cache built unseeded is not comparable to a diff --git a/tests/test_variant_node_sharing.py b/tests/test_variant_node_sharing.py index 932bbc9..d2e443d 100644 --- a/tests/test_variant_node_sharing.py +++ b/tests/test_variant_node_sharing.py @@ -1,17 +1,22 @@ -"""LNG_SHARE_VARIANT_NODES: the same entity in the same role across the variants of +"""Variant-node sharing: the same entity in the same role across the variants of ONE Reactome reaction shares one UUID; across different reactions it does not. +Unconditional since deltasignal specs/020 (nodes -34.6%, edges -21.5%, no +pathway gains cyclic nodes, held-out net zero on both ground-truth axes). The +LNG_SHARE_VARIANT_NODES flag that gated it is gone, and setting it is an error. + Motivation (deltasignal specs/016): HDR's BCDX2 complex existed as 33 node copies -- one per variant reaction -- and no solver rule over copies is right: min over them measured -83 held-out, max -117. One node is the fix. """ + from src.logic_network_generator import _register_phase1 -def phase1(vr_entities, vr_to_reaction, share, roots=frozenset(), terms=frozenset()): +def phase1(vr_entities, vr_to_reaction, roots=frozenset(), terms=frozenset()): reg = {} _register_phase1(vr_entities, reg, set(roots), {}, set(terms), {}, - vr_to_reaction=vr_to_reaction, share_variants=share) + vr_to_reaction=vr_to_reaction) return reg @@ -21,31 +26,37 @@ def phase1(vr_entities, vr_to_reaction, share, roots=frozenset(), terms=frozense V2R = {"v1": "R1", "v2": "R1", "v3": "R2"} -def test_off_every_variant_gets_its_own_uuid(): - reg = phase1(VR, V2R, share=False) - assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] - assert reg[("A", "v1", "input")] != reg[("A", "v2", "input")] +# The guard rejecting a stale LNG_SHARE_VARIANT_NODES is tested for real in +# tests/test_removed_env_flags.py, which calls it and drives the entry point. +# An earlier version asserted on inspect.getsource() here; that passed whether +# or not the guard ran, and broke when the guard moved to a shared helper. + + +def test_sharing_is_on_with_no_flag_set(): + reg = phase1(VR, V2R) + assert reg[("B", "v1", "output")] == reg[("B", "v2", "output")] + assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] def test_on_same_entity_same_reaction_same_role_shares_one_uuid(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("B", "v1", "output")] == reg[("B", "v2", "output")] assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] def test_on_different_reactions_stay_distinct(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("B", "v1", "output")] != reg[("B", "v3", "output")] # positional across reactions, as designed def test_on_roles_are_not_conflated(): vr = {"v1": (["B"], ["B"])} # B both consumed and produced by one variant - reg = phase1(vr, {"v1": "R1"}, share=True) + reg = phase1(vr, {"v1": "R1"}) assert reg[("B", "v1", "input")] != reg[("B", "v1", "output")] def test_on_variant_specific_members_are_not_conflated(): - reg = phase1(VR, V2R, share=True) + reg = phase1(VR, V2R) assert reg[("S1", "v1", "input")] != reg[("S2", "v2", "input")] # different entities assert reg[("P1", "v1", "output")] != reg[("P2", "v2", "output")] @@ -53,25 +64,47 @@ def test_on_variant_specific_members_are_not_conflated(): def test_boundary_entities_are_left_to_their_own_caches(): # "A" is a root input: the boundary cache already shares it per stId; sharing # must not create a second uuid for it. - reg = phase1(VR, V2R, share=True, roots={"A"}) + # + # NOTE on what this can and cannot catch. The `eid not in boundary_eids` + # skip in _register_phase1 is a STATS/CLARITY guard, not a correctness one: + # _register_entity_uuid consults the boundary cache regardless, so the + # variant cache could only ever store the boundary uuid. Deleting the skip + # leaves every uuid identical (verified over 300 randomized inputs) and only + # changes the `shared`/`distinct` counters. So assert on those too, or this + # test passes with the skip removed. + reg = {} + stats = _register_phase1(VR, reg, {"A"}, {}, set(), {}, vr_to_reaction=V2R) assert reg[("A", "v1", "input")] == reg[("A", "v2", "input")] assert len({reg[("A", v, "input")] for v in ("v1", "v2")}) == 1 + # "A" must not be counted as a variant-cache reuse: the boundary cache did it. + assert stats["shared"] == 1, stats # only B in R1 shares; A is boundary-excluded + assert stats["distinct"] == 7, stats def test_terminal_output_shared_across_reactions_by_its_own_cache_not_the_variant_cache(): # "B" is an output in R1 (v1, v2) AND in R2 (v3), and a terminal output. The - # boundary cache shares it per stId across ALL three; an implementation that - # ignored the boundary exclusion and shared via the variant cache would give - # v3 a different uuid from v1/v2. All three must be one uuid. - reg = phase1(VR, V2R, share=True, terms={"B"}) + # boundary cache shares it per stId across ALL three, so all three must be + # one uuid. + # + # The previous comment here claimed that "an implementation that ignored the + # boundary exclusion and shared via the variant cache would give v3 a + # different uuid from v1/v2". That mechanism cannot occur -- the variant + # cache would store the same boundary uuid -- so the claim was asserting + # something unreachable. The uuid assertion below is still the property we + # want; the stats assertion is what actually distinguishes the two + # implementations. + reg = {} + stats = _register_phase1(VR, reg, set(), {}, {"B"}, {}, vr_to_reaction=V2R) assert len({reg[("B", v, "output")] for v in ("v1", "v2", "v3")}) == 1 + assert stats["shared"] == 1, stats # A in R1; B is boundary-excluded + assert stats["distinct"] == 6, stats def test_missing_reaction_id_read_back_as_nan_string_is_not_a_shared_key(): # reactome_id read from a cached CSV with no dtype turns NaN into "nan"; # every variant of every reaction with a missing id must NOT collapse onto # one uuid through the key (eid, "nan", role). - reg = phase1(VR, {"v1": "nan", "v2": "nan", "v3": "nan"}, share=True) + reg = phase1(VR, {"v1": "nan", "v2": "nan", "v3": "nan"}) assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] assert reg[("B", "v1", "output")] != reg[("B", "v3", "output")] @@ -80,12 +113,12 @@ def test_partial_reaction_map_is_reported(): from src.logic_network_generator import _register_phase1 reg = {} stats = _register_phase1(VR, reg, set(), {}, set(), {}, - vr_to_reaction={"v1": "R1", "v3": "R2"}, share_variants=True) + vr_to_reaction={"v1": "R1", "v3": "R2"}) assert stats["unmapped"] == 1 # the unmapped variant is registered per-variant, the mapped ones still share among themselves assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")] def test_unknown_reaction_falls_back_to_per_variant(): - reg = phase1(VR, {}, share=True) # no vr -> reaction map + reg = phase1(VR, {}) # no vr -> reaction map assert reg[("B", "v1", "output")] != reg[("B", "v2", "output")]