Variant-node sharing is now unconditional (nodes -34.6%, accuracy neutral)

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`). |

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,27 +512,47 @@ 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).
 
     A Reactome reaction with EntitySet participants becomes several VIRTUAL
     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")

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

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,57 +26,85 @@ 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")]
 
 
 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")]