Fix #67: context export named parent uuids that are not in the network

src/logic_network_generator.py

@@ -1,5 +1,6 @@
 import os
 import uuid
+from collections import Counter
 from typing import Dict, List, Any, NamedTuple, Optional, Set, Tuple
 
 import pandas as pd
@@ -2359,8 +2360,26 @@ def export_nodes(pathway_logic_network: pd.DataFrame,
 def export_node_reaction_context(entity_uuid_registry: Dict[tuple, str],
                                  reaction_id_map: pd.DataFrame,
                                  catalyst_regulator_map: pd.DataFrame,
-                                 output_file: str) -> None:
-    """Write node_reaction_context.csv — (node, reaction, role) location rows."""
+                                 output_file: str,
+                                 logic_network: Optional[pd.DataFrame] = None) -> None:
+    """Write node_reaction_context.csv — (node, reaction, role) location rows.
+
+    Catalyst and regulator rows are read from the emitted network, NOT from
+    ``catalyst_regulator_map``. Those fetch rows carry the UNDECOMPOSED PARENT
+    entity's uuid, while ``append_regulators`` decomposes each catalyst and
+    regulator to its terminal members and wires the edges from the member
+    uuids. Exporting the parent therefore named a node that does not exist:
+    measured on the ten-pathway catalog, 100% of 2,228 catalyst rows and 100%
+    of 1,018 regulator rows were orphaned — 31.3% of the export, and exactly
+    the two roles that carry the causality — while input and output were
+    clean at 0%. That is issue #67.
+
+    Deriving these rows from the network instead makes the export correct by
+    construction: it reports what was actually wired rather than what was
+    fetched. Catalyst and regulator edges carry no ``edge_reaction_id``, but
+    their TARGET is the virtual-reaction node, so the reaction is recovered
+    through ``reaction_id_map`` the same way the input/output rows do it.
+    """
     vr_to_reaction = dict(zip(reaction_id_map["uid"].astype(str),
                               reaction_id_map["reactome_id"].astype(str)))
     seen: Set[tuple] = set()
@@ -2376,17 +2395,47 @@ def export_node_reaction_context(entity_uuid_registry: Dict[tuple, str],
         seen.add(key)
         rows.append({"context_node": str(node_uuid), "reaction_id": rid, "role": role})
 
-    if catalyst_regulator_map is not None and not catalyst_regulator_map.empty:
-        for _, r in catalyst_regulator_map.iterrows():
-            cr_uuid = r.get("uuid"); rid = r.get("reaction_id"); et = str(r.get("edge_type"))
-            if pd.isna(cr_uuid) or pd.isna(rid):
+    has_cr = catalyst_regulator_map is not None and not catalyst_regulator_map.empty
+    if has_cr and logic_network is None:
+        # Falling back to the fetch rows here would silently restore #67.
+        raise ValueError(
+            "export_node_reaction_context needs the logic network to place "
+            "catalyst and regulator nodes; the catalyst_regulator_map holds "
+            "undecomposed parent uuids that are absent from the network (#67)."
+        )
+    if logic_network is not None and not logic_network.empty:
+        for _, e in logic_network.iterrows():
+            role = str(e.get("edge_type") or "")
+            if role not in ("catalyst", "regulator"):
                 continue
-            role = "catalyst" if et == "catalyst" else "regulator"
-            key = (str(cr_uuid), str(rid), role)
+            # Distinct names from the registry loop above, whose `node_uuid`
+            # and `rid` are already narrowed to str; rebinding them with
+            # DataFrame values (Any | None) is a type error, not a style
+            # preference.
+            edge_node = e.get("source_id")
+            edge_reaction = vr_to_reaction.get(str(e.get("target_id")))
+            if pd.isna(edge_node) or edge_reaction is None:
+                continue
+            key = (str(edge_node), edge_reaction, role)
             if key in seen:
                 continue
             seen.add(key)
-            rows.append({"context_node": str(cr_uuid), "reaction_id": str(rid), "role": role})
+            rows.append({"context_node": str(edge_node),
+                         "reaction_id": edge_reaction, "role": role})
+
+    # A context row naming a node that is not in the network is meaningless to
+    # every consumer, so refuse to write one rather than shipping it quietly.
+    if logic_network is not None and not logic_network.empty:
+        live = set(logic_network["source_id"].astype(str)) | set(
+            logic_network["target_id"].astype(str))
+        orphaned = [r for r in rows if r["context_node"] not in live]
+        if orphaned:
+            by_role = Counter(r["role"] for r in orphaned)
+            raise ValueError(
+                f"{len(orphaned)} of {len(rows)} context rows name nodes absent "
+                f"from the logic network (by role: {dict(by_role)}). Refusing to "
+                f"write {output_file}."
+            )
 
     cols = ["context_node", "reaction_id", "role"]
     pd.DataFrame(rows, columns=cols).to_csv(output_file, index=False)

src/pathway_generator.py

@@ -400,6 +400,7 @@ def generate_pathway_file(
                 result.reaction_id_map,
                 result.catalyst_regulator_map,
                 str(pathway_output_dir / "node_reaction_context.csv"),
+                logic_network=result.logic_network,
             )
         except Exception as e:
             logger.error(f"Failed to write node provenance files: {e}", exc_info=True)

tests/test_provenance_exports.py

@@ -46,22 +46,103 @@ def test_export_nodes_classifies_kinds(tmp_path, monkeypatch):
 
 
 def test_export_node_reaction_context(tmp_path):
+    """Catalyst rows name the wired member, not the fetch row's parent uuid.
+
+    This test previously asserted the OPPOSITE — that the parent uuid "c1"
+    from the catalyst fetch row appears in the export. That expectation is
+    what kept #67 alive: the export was orphaning 100% of catalyst and
+    regulator rows and a green test said it was correct. Kept here, inverted,
+    as the record of what the right answer is.
+    """
     entity_uuid_registry = {
         ("R-HSA-999", "rxn1", "input"): "s1",
         ("R-HSA-141608::variant::x", "rxn1", "output"): "v1",
     }
     reaction_id_map = pd.DataFrame({"uid": ["rxn1"], "reactome_id": ["R-HSA-100"]})
+    # "c1" is the undecomposed parent the fetch row carries; "c1_member" is the
+    # terminal member append_regulators actually wires to the reaction.
     catreg = pd.DataFrame([{"reaction_id": "R-HSA-100", "entity_id": "R-HSA-7",
                             "edge_type": "catalyst", "uuid": "c1", "reaction_uuid": "rxn1"}])
+    edges = pd.DataFrame([
+        {"source_id": "s1", "target_id": "rxn1", "pos_neg": "pos", "and_or": "and",
+         "edge_type": "input", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"},
+        {"source_id": "rxn1", "target_id": "v1", "pos_neg": "pos", "and_or": None,
+         "edge_type": "output", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"},
+        {"source_id": "c1_member", "target_id": "rxn1", "pos_neg": "pos", "and_or": "and",
+         "edge_type": "catalyst", "stoichiometry": 1, "edge_reaction_id": None},
+    ])
     out = tmp_path / "ctx.csv"
-    m.export_node_reaction_context(entity_uuid_registry, reaction_id_map, catreg, str(out))
+    m.export_node_reaction_context(entity_uuid_registry, reaction_id_map, catreg,
+                                   str(out), logic_network=edges)
     ctx = pd.read_csv(out).to_dict("records")
     triples = {(r["context_node"], r["reaction_id"], r["role"]) for r in ctx}
     assert ("s1", "R-HSA-100", "input") in triples
     assert ("v1", "R-HSA-100", "output") in triples
-    assert ("c1", "R-HSA-100", "catalyst") in triples
+    assert ("c1_member", "R-HSA-100", "catalyst") in triples
+    assert ("c1", "R-HSA-100", "catalyst") not in triples
 
 
 def test_parse_variant_members():
     assert m._parse_variant_members("R-HSA-1::variant::R-HSA-2_R-HSA-3") == {"R-HSA-2", "R-HSA-3"}
     assert m._parse_variant_members("R-HSA-1") == set()
+
+
+def test_context_export_emits_no_orphaned_rows(tmp_path):
+    """Every context row must name a node that exists in the logic network.
+
+    LNG #67: append_regulators DECOMPOSES a catalyst or regulator to its
+    terminal members and emits edges from those member UUIDs, but the export
+    wrote the UUID carried on the raw catalyst/regulator fetch row — the
+    UNDECOMPOSED PARENT. Measured on the ten-pathway catalog before the fix:
+    100% of 2,228 catalyst rows and 100% of 1,018 regulator rows named a node
+    absent from logic_network.csv, 31.3% of the whole export, while input and
+    output were clean at 0%.
+
+    That silently broke every consumer that tries to answer "which node is
+    this entity, at this reaction, in this role" for exactly the two roles
+    that carry the causality.
+    """
+    rxn1 = "aaaaaaaa-0000-0000-0000-0000000000r1"
+    inp = "aaaaaaaa-0000-0000-0000-000000000001"
+    out_uuid = "aaaaaaaa-0000-0000-0000-000000000002"
+    # The decomposed member the network actually wires up...
+    member = "aaaaaaaa-0000-0000-0000-00000000000c"
+    # ...versus the parent complex the fetch row carries. Distinct on purpose:
+    # when they coincide the bug is invisible.
+    parent = "aaaaaaaa-0000-0000-0000-0000000000ff"
+
+    edges = pd.DataFrame([
+        {"source_id": inp, "target_id": rxn1, "pos_neg": "pos", "and_or": "and",
+         "edge_type": "input", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"},
+        {"source_id": rxn1, "target_id": out_uuid, "pos_neg": "pos", "and_or": None,
+         "edge_type": "output", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"},
+        {"source_id": member, "target_id": rxn1, "pos_neg": "pos", "and_or": "and",
+         "edge_type": "catalyst", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"},
+    ])
+    reaction_id_map = pd.DataFrame({"uid": [rxn1], "reactome_id": ["R-HSA-100"]})
+    entity_uuid_registry = {
+        ("R-HSA-11", rxn1, "input"): inp,
+        ("R-HSA-22", rxn1, "output"): out_uuid,
+    }
+    catalyst_regulator_map = pd.DataFrame([
+        {"uuid": parent, "reaction_id": "R-HSA-100", "edge_type": "catalyst"},
+    ])
+
+    out = tmp_path / "node_reaction_context.csv"
+    m.export_node_reaction_context(entity_uuid_registry, reaction_id_map,
+                                   catalyst_regulator_map, str(out),
+                                   logic_network=edges)
+
+    rows = pd.read_csv(out, dtype=str, keep_default_na=False).to_dict("records")
+    live = set(edges["source_id"]) | set(edges["target_id"])
+    orphans = [r for r in rows if r["context_node"] not in live]
+    assert not orphans, f"orphaned context rows: {orphans}"
+
+    # And the catalyst must be present at all — dropping the row instead of
+    # fixing the UUID would also make the orphan count zero.
+    catalysts = [r for r in rows if r["role"] == "catalyst"]
+    assert len(catalysts) == 1, f"expected one catalyst row, got {catalysts}"
+    assert catalysts[0]["context_node"] == member, (
+        "catalyst row must name the decomposed member the network wires up, "
+        f"not the parent fetch-row UUID; got {catalysts[0]['context_node']}"
+    )