From 7c25f58fe641ba18a57a745438d32661ea832862 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 16:23:16 -0400 Subject: [PATCH 1/2] Fix #67: context export named parent uuids that are not in the network MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit append_regulators decomposes each catalyst and regulator to its terminal members and wires the edges from those member uuids. The context export wrote the uuid carried on the raw catalyst/regulator fetch row instead — the undecomposed parent — so it named nodes that do not exist. Measured on the ten-pathway catalog before the fix: input 3,641 rows 0 orphaned 0.0% output 3,491 rows 0 orphaned 0.0% catalyst 2,228 rows 2,228 orphaned 100.0% regulator 1,018 rows 1,018 orphaned 100.0% TOTAL 10,378 3,246 31.3% Every consumer asking "which node is this entity, at this reaction, in this role" got a wrong answer for exactly the two roles that carry the causality. Catalyst and regulator rows are now read from the emitted network rather than from the fetch rows, which makes the export correct by construction: it reports what was wired, not what was requested. Those edges carry no edge_reaction_id, but their target IS the virtual-reaction node, so the reaction is recovered through reaction_id_map exactly as the input/output rows already do. Two guards, because the quiet failure is what made this survive: - the export refuses to write when a row names a node absent from the network, and reports the count by role rather than shipping it; - omitting the network while catalyst rows exist now raises, instead of falling back to the fetch rows and silently restoring the bug. The bug was held in place by a test. test_export_node_reaction_context asserted that the parent uuid "c1" appears in the export — the exact wrong behaviour — so the suite was green while a third of the export was broken. That test is inverted and kept as the record, alongside a new one that fails when the network and the export disagree. Verified as a negative control: reintroducing the parent uuid makes the new test fail, restoring the fix makes it pass. 167 tests pass in the no-database tier. Found while specifying deltasignal's 005-node-identity-mapping, where the same 31.3% turns out to be 25 of the 42 diagram-glyph join misses. Co-Authored-By: Claude Opus 5 (1M context) --- src/logic_network_generator.py | 62 +++++++++++++++++++---- src/pathway_generator.py | 1 + tests/test_provenance_exports.py | 85 +++++++++++++++++++++++++++++++- 3 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 44c8594..3ab1c82 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,42 @@ 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) + node_uuid = e.get("source_id") + rid = vr_to_reaction.get(str(e.get("target_id"))) + if pd.isna(node_uuid) or rid is None: + continue + key = (str(node_uuid), rid, 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(node_uuid), "reaction_id": rid, "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']}" + ) From 081ec9cbefe35904a1f88df366bfef81c025d44d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 14 Sep 2026 12:46:01 -0400 Subject: [PATCH 2/2] Fix mypy: the network loop rebound names already narrowed to str MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI runs mypy on src/ and I only ran pytest locally, so this failed on all three Python versions with a green local run. The catalyst/regulator loop reused node_uuid and rid, both of which the registry loop above binds as str from Dict[tuple, str]. Rebinding them with DataFrame values, which are Any | None, is an assignment type error. Renamed to edge_node and edge_reaction, which also reads better — they are a different thing from the registry's entries. Verified locally with the full CI command set this time: mypy clean on 10 files, ruff clean, 167 tests passing at 46.79% coverage against the 40% floor. Co-Authored-By: Claude Opus 5 (1M context) --- src/logic_network_generator.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 3ab1c82..09fb03a 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -2408,15 +2408,20 @@ def export_node_reaction_context(entity_uuid_registry: Dict[tuple, str], role = str(e.get("edge_type") or "") if role not in ("catalyst", "regulator"): continue - node_uuid = e.get("source_id") - rid = vr_to_reaction.get(str(e.get("target_id"))) - if pd.isna(node_uuid) or rid is None: + # 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(node_uuid), rid, role) + key = (str(edge_node), edge_reaction, role) if key in seen: continue seen.add(key) - rows.append({"context_node": str(node_uuid), "reaction_id": 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.