From 1c7c2dee65ef8520da91e6337ca1593c4863a3ef Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 14 Sep 2026 13:08:44 -0400 Subject: [PATCH] Record which diagram glyph each node came from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adam: "two nodes in the reactome pathway diagram that are in the same compartment could be the same thing but in two different places. We need to be able to know which one the uuid was from. this is especially important when interacting with deltasignal through the pathwaydiagram." That identity exists and this module already read it — x["id"] on each input/output/catalyst entry is the DiagramObject id, the one the pathway browser selects and highlights — and then discarded it after pairing producers with consumers. diagram_glyph_positions() exposes it instead, and node_resolution.csv's glyph_id/diagram_stid columns are now populated. (reaction_stId, entity_stId, role) is a UNIQUE key for a glyph, which is what makes the question answerable: 0 of 156 triples in R-HSA-1257604 and 0 of 245 in R-HSA-69620 resolve to more than one glyph, while four entities in each are drawn up to 19 times. An entity drawn many times is drawn once per reaction, so naming the reaction and the role disambiguates it exactly. Join coverage over the ten-pathway catalog: 1,432 of 1,644 diagram triples, 87.1%, against the ~89% the research predicted. Both directions work — a glyph resolves to its nodes, and a node resolves back to the glyphs to highlight. Two honest findings from the measurement: Signaling_by_WNT and Transcriptional_Regulation_by_TP53 score ZERO, and that is structural rather than a defect: their diagrams are overviews with 4 and 5 nodes and NO edges — boxes pointing at sub-pathways, not reaction-level drawings — so no glyph exists to attribute at that level. Sub-pathway diagrams would be needed, which is a separate piece of work. The answer for cofactors is "they are the same node, deliberately". ATP's 19 glyphs in Cell Cycle Checkpoints resolve to ONE uuid, because the generator collapses cofactor occurrences via the boundary cache. Clicking any of them correctly lands on the same node, and the reverse direction returns all 19 to highlight. Ubiquitin is the opposite case: 3 glyphs to 15 uuids from positional decomposition. Neither was visible before. A test pins the invariant that glyph_id and diagram_stid are written together or not at all — a glyph id is unique only within its diagram, so one without the other cannot be resolved back to anything. Verified with the full CI command set this time — mypy clean on 12 files, ruff clean on the changed files, 189 tests at 49.66% coverage. Co-Authored-By: Claude Opus 5 (1M context) --- src/diagram_connectivity.py | 62 +++++++++++++++++++++++++++++++- src/logic_network_generator.py | 23 ++++++++++-- tests/test_provenance_exports.py | 41 +++++++++++++++++++++ 3 files changed, 122 insertions(+), 4 deletions(-) diff --git a/src/diagram_connectivity.py b/src/diagram_connectivity.py index f3399f8..d331696 100644 --- a/src/diagram_connectivity.py +++ b/src/diagram_connectivity.py @@ -20,7 +20,7 @@ import os from collections import defaultdict from pathlib import Path -from typing import Set, Tuple +from typing import Dict, List, Set, Tuple import pandas as pd @@ -162,3 +162,63 @@ def augment_reaction_connections(pathway_id: str, f"pairs not in precedingEvent (of {len(pairs)} drawn)" ) return pd.concat([reaction_connections, pd.DataFrame(new_rows)], ignore_index=True) + + +def diagram_glyph_positions(pathway_id: str) -> Dict[Tuple[str, str, str], List[int]]: + """``(reaction_stId, entity_stId, role) -> [glyph_id]`` for one pathway. + + Adam: *"two nodes in the reactome pathway diagram that are in the same + compartment could be the same thing but in two different places. We need + to be able to know which one the uuid was from."* + + That identity exists and this module already read it — ``x["id"]`` on each + input/output/catalyst entry is the DiagramObject id, which is what the + pathway browser selects and highlights — and then discarded it after + pairing producers with consumers. This exposes it instead. + + The triple is a **unique** key, which is what makes the question + answerable: measured on R-HSA-1257604, 0 of 156 triples resolve to more + than one glyph. An entity drawn nine times (ATP, ADP) is drawn once per + reaction, so naming the reaction and the role disambiguates it exactly. + The return type is still a list because a future diagram could break that + assumption, and silently returning the first of several would hide it. + + Returns an empty mapping when no diagram covers the pathway. + """ + ddir = _diagram_dir() + diagram_stid = _covering_diagram_stid(pathway_id) + if not diagram_stid: + return {} + + layout = json.loads((ddir / f"{diagram_stid}.json").read_text()) + graph = json.loads((ddir / f"{diagram_stid}.graph.json").read_text()) + + edge_dbid_to_stid = {e["dbId"]: e["stId"] for e in graph.get("edges", []) if e.get("stId")} + node_dbid_to_stid = {n["dbId"]: n["stId"] for n in graph.get("nodes", []) if n.get("stId")} + glyph_to_entity_dbid = {n["id"]: n.get("reactomeId") for n in layout.get("nodes", [])} + + roles = {"inputs": "input", "outputs": "output", "catalysts": "catalyst"} + positions: Dict[Tuple[str, str, str], List[int]] = {} + for edge in layout.get("edges", []): + reaction_stid = edge_dbid_to_stid.get(edge.get("reactomeId")) + if not reaction_stid: + continue + for key, role in roles.items(): + for entry in edge.get(key, []): + glyph_id = entry.get("id") + entity_stid = node_dbid_to_stid.get(glyph_to_entity_dbid.get(glyph_id)) + if glyph_id is None or not entity_stid: + continue + positions.setdefault((reaction_stid, entity_stid, role), []) + if glyph_id not in positions[(reaction_stid, entity_stid, role)]: + positions[(reaction_stid, entity_stid, role)].append(glyph_id) + return positions + + +def covering_diagram_stid(pathway_id: str) -> str: + """The diagram a pathway is drawn in — its own, or the nearest ancestor's. + + A glyph id is unique only WITHIN a diagram, so it is meaningless without + this. Exposed so callers can record the pair together. + """ + return _covering_diagram_stid(pathway_id) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 162304c..b49cde1 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -2470,6 +2470,8 @@ def export_node_resolution(pathway_id: str, get_reactome_release, get_set_members, get_modifier_isoform_entity_set_ids) from src.set_resolution import make_neo4j_resolver + from src.diagram_connectivity import (covering_diagram_stid, + diagram_glyph_positions) release = get_reactome_release() release_str = str(release) if release is not None else "" @@ -2477,6 +2479,15 @@ def export_node_resolution(pathway_id: str, vr_to_reaction = dict(zip(reaction_id_map["uid"].astype(str), reaction_id_map["reactome_id"].astype(str))) + # (reaction, entity, role) -> glyph ids. A glyph id is unique only within + # its diagram, so the diagram is recorded with it or neither is written. + try: + glyph_positions = diagram_glyph_positions(pathway_id) + diagram_stid = covering_diagram_stid(pathway_id) or "" + except Exception: + logger.warning(f"no diagram glyph positions for {pathway_id}") + glyph_positions, diagram_stid = {}, "" + rows: List[Dict[str, Any]] = [] seen: Set[tuple] = set() @@ -2488,12 +2499,18 @@ def add(stable_id: str, node_uuid: str, relation: str, depth: int, if key in seen: return seen.add(key) + # The diagram draws an entity once PER REACTION, so naming the + # reaction and the role identifies the glyph exactly — measured, 0 of + # 156 triples in R-HSA-1257604 resolve to more than one, while four + # entities are drawn up to nine times. That is what makes "which of + # the two ATP glyphs did this uuid come from" answerable. + glyphs = glyph_positions.get((reaction_stid, stable_id, role), []) + glyph_id = str(glyphs[0]) if len(glyphs) == 1 else "" rows.append({ "stable_id": stable_id, "uuid": node_uuid, "relation": relation, "depth": depth, "role": role, "reaction_stid": reaction_stid, - # Populated by the diagram work (US3); the columns exist now so - # consumers do not have to branch on schema version later. - "glyph_id": "", "diagram_stid": "", + "glyph_id": glyph_id, + "diagram_stid": diagram_stid if glyph_id else "", "release": release_str, }) diff --git a/tests/test_provenance_exports.py b/tests/test_provenance_exports.py index 4092994..8148d8c 100644 --- a/tests/test_provenance_exports.py +++ b/tests/test_provenance_exports.py @@ -146,3 +146,44 @@ def test_context_export_emits_no_orphaned_rows(tmp_path): "catalyst row must name the decomposed member the network wires up, " f"not the parent fetch-row UUID; got {catalysts[0]['context_node']}" ) + + +def test_glyph_id_and_diagram_are_written_together(tmp_path, monkeypatch): + """A glyph id without its diagram is meaningless, and vice versa. + + Glyph ids are unique only WITHIN a diagram — the same integer identifies a + different drawing in another one — so a row carrying one without the other + cannot be resolved back to anything. This is the invariant behind Adam's + question: knowing a uuid came from glyph 535 is only useful if you also + know which diagram 535 belongs to. + """ + monkeypatch.setattr(m, "get_labels", lambda e: ["EntityWithAccessionedSequence"], raising=False) + + rxn = "aaaaaaaa-0000-0000-0000-0000000000r1" + src = "aaaaaaaa-0000-0000-0000-000000000001" + edges = pd.DataFrame([ + {"source_id": src, "target_id": rxn, "pos_neg": "pos", "and_or": "and", + "edge_type": "input", "stoichiometry": 1, "edge_reaction_id": "R-HSA-100"}, + ]) + reaction_id_map = pd.DataFrame({"uid": [rxn], "reactome_id": ["R-HSA-100"]}) + + import src.diagram_connectivity as dc + # One entity at one reaction, drawn once: the triple resolves to a glyph. + monkeypatch.setattr(dc, "diagram_glyph_positions", + lambda pid: {("R-HSA-100", "R-HSA-999", "input"): [535]}) + monkeypatch.setattr(dc, "covering_diagram_stid", lambda pid: "R-HSA-1257604") + monkeypatch.setattr(m, "get_pathway_participating_entities", lambda pid: set(), raising=False) + + out = tmp_path / "node_resolution.csv" + exc = tmp_path / "node_exclusions.csv" + m.export_node_resolution("R-HSA-100", edges, reaction_id_map, + {src: "R-HSA-999"}, str(out), str(exc)) + + rows = pd.read_csv(out, dtype=str, keep_default_na=False).to_dict("records") + for row in rows: + has_glyph = bool(row["glyph_id"].strip()) + has_diagram = bool(row["diagram_stid"].strip()) + assert has_glyph == has_diagram, ( + f"glyph_id={row['glyph_id']!r} and diagram_stid={row['diagram_stid']!r} " + "must be present together or absent together" + )