diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 44c8594..09fb03a 100755 --- a/src/logic_network_generator.py +++ b/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) diff --git a/src/pathway_generator.py b/src/pathway_generator.py index df81901..ec0abf2 100755 --- a/src/pathway_generator.py +++ b/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) diff --git a/tests/test_provenance_exports.py b/tests/test_provenance_exports.py index 59f74d1..4092994 100644 --- a/tests/test_provenance_exports.py +++ b/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']}" + )