Derive the cofactor set, and carry the realisation links the diagram draws - #80
Merged
Merged
Conversation
…draws Two defects found by auditing the generated networks against Reactome and against the pathway diagrams. Both verified before fixing; several other apparent defects did NOT survive that check and are not touched here. DERIVED COFACTOR SET. `_COFACTOR_STIDS` had six of thirteen entries wrong against Release97: R-ALL-29438 commented "PPi" is GTP, R-ALL-29390 commented "Pi variant" is PXLP, R-ALL-29360 commented "ADP variant" is NAD+, and R-ALL-217093 / R-ALL-110114 / R-ALL-29986 do not exist at all. The effect was not cosmetic — PPi, several NAD(P)(H) forms and a Pi variant were never excluded from depletion edges, handoff leaves or diagram bridges, while PXLP was excluded and should not have been. `_cofactor_stids()` now derives the set by ChEBI identity from the connected release (253 species at Release97) via the existing `get_cofactor_species`, with a small verified seed as the offline fallback. A fallback that returned an empty set would silently stop excluding cofactors everywhere, so the seed is asserted non-empty. MEASURED: no behavioural change. Regenerated six pathways including every one where a species whose exclusion status the fix changes is both produced and consumed (TP53, PTK6, RAF, PIP3, S_Phase, Mitotic_Prophase) — byte-identical edge counts. The wrongly-listed species simply never arise as bridge or depletion candidates. Landed to stop the list rotting further, not for accuracy. DIAGRAM SET-MEMBER LINKS. `diagram_connectivity` read the layout JSON but used only its `edges`, never `layout["links"]`. Most of those restate set membership Neo4j already has, but not all: `SMAD7:SMURF2` and `SMAD7:SMURF/NEDD4L` are both curated as COMPLEXES with no containment between them — the generic holds a SMURF/NEDD4L DefinedSet where the specific holds SMURF2 — and the diagram joins them with an EntitySetAndMemberLink. The generated network had them as two unconnected nodes. Same shape for the EPH-ephrin oligomer complexes. `diagram_set_member_pairs` returns those pairs member-first and `_emit_diagram_set_member_edges` adds one `pos`/`or` edge per pair. ONE edge per stable-id pair, not the cartesian product of occurrences: all-pairs turned two curated relationships into 48 edges in EPH-Ephrin alone, which is the blow-up that made the all-pairs silo bridge unusable. Only EntitySetAndMemberLink is consumed; Interaction and set-to-set links point at entities with no node here, so using them means adding nodes (#41). Adds 5 edges across the catalog: 3 in TGF-beta, 2 in EPH-Ephrin. Gated on LNG_DIAGRAM_SET_MEMBER, default on. mypy and ruff clean. pytest 984 passed; the single failure (test_main_edges_proportional_to_best_matches[Class_I_MHC...]) reproduces on a stashed tree and is unrelated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-caught before merge. The edge is correct in principle and inert in
practice, for two compounding reasons.
The target already carries `and` assembly edges to every constituent protein —
for SMAD7:SMURF/NEDD4L those are SMAD7, SMURF1, SMURF2 and NEDD4L — so the
new `or` edge lands beside an AND cluster. DeltaSignal combines the two as
max(and_result, or_result) by default, which discards any OR-cluster LOSS.
Measured on the regenerated TGF-beta network:
DS_OR_COMBINE=max members DOWN -> generic 1.0 (change discarded)
members UP -> generic 80.0
DS_OR_COMBINE=gate members DOWN -> generic 0.0
members UP -> generic 0.2082
An edge that propagates increases but not decreases is worse than no edge,
because it looks like it works and silently biases every downstream analysis.
It becomes load-bearing under DS_OR_COMBINE=gate — which was itself measured
completely inert (0 of 21,450 predictions, 0 raw values) precisely because
nothing in the catalog produced a reaction with both and and or activators.
This produces them. So the two have to be enabled and measured together, and
neither is worth measuring alone. Note `gate` on the UP direction gives 0.2082,
below baseline, which is its own open question.
The extraction, the emitter and the tests stay; only the default changes, so
generated networks are unaffected until someone deliberately opts in.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…erminism Ten findings from an adversarial review of the previous two commits. The first is a regression I introduced and is the one that mattered. SEED REGRESSION (the review's finding 1). The new seed dropped R-ALL-29438 and R-ALL-29360 because their COMMENTS were wrong — "PPi" and "ADP variant". But both are genuine cofactors: GTP and NAD+ are in `_COFACTOR_CHEBI`, so they were mis-labelled, not mis-included. Dropping them made the offline fallback exclude FEWER real cofactors than the 13-entry list it replaced, which is exactly the failure the change claims to fix. Only PXLP was a true false positive; the three absent ids were inert. Restored, and the test now asserts the distinction instead of cementing the error. LATCHING CACHE (finding 3). `_cofactor_stids` memoised the FAILURE, so one transient reset on pathway 1 would build pathways 2..N with the seed instead of 253 species — while `export_cofactors` queries Neo4j separately, succeeds, and ships a cofactors.csv listing all 253 beside a network built without them. No caller could detect the mismatch. Now only success is cached, matching `get_cofactor_species`, and the warning says the networks are not comparable. NONDETERMINISM (finding 5). The tie-break `max(..., key=(degree, uuid))` reads a uuid4 that is regenerated every run, so tied occurrences won differently run to run — defeating the reproducibility the pinned PYTHONHASHSEED exists to provide. Ties now break on insertion order. Also: scope borrowed-diagram links to this pathway's own entities, so a sub-pathway without its own diagram no longer imports a sibling's realisation links (6); honour LNG_DIAGRAM_CONNECTIVITY=0 so the documented kill switch is true again, and fix the comment that claimed it already was (7); add LNG_DIAGRAM_SET_MEMBER to _FINGERPRINTED_ENV so two A/B arms are distinguishable afterwards (8); hoist the bridge exclusion set out of a loop that rebuilt a ~256-element union per triple (10); add a fixture clearing _cofactor_stids_cache and _handoff_leaf_cache on failure as well as success, and a test that fails if the emitter is unwired (12). Recorded, not fixed: the "no behavioural change" measurement covered six SIGNALLING pathways, and H+/CoA-SH/AdoMet/redox pairs are load-bearing in metabolic ones (11); the `or` edge also masks genuine subunit knockouts of the generic complex, not just discards member losses (4) — both are reasons the feature stays default-off. mypy and ruff clean. pytest 986 passed; the single failure reproduces on a stashed tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects found by auditing the generated networks against Reactome and against the pathway diagrams. Both verified before fixing — several other apparent defects did not survive that check and are deliberately not touched.
1. The cofactor list had 6 of 13 entries wrong
Verified against Release97:
R-ALL-29438R-ALL-29390R-ALL-29360R-ALL-217093R-ALL-110114R-ALL-29986Not cosmetic: PPi, several NAD(P)(H) forms and a Pi variant were never excluded from depletion edges, handoff leaves or diagram bridges, while PXLP was excluded and shouldn't have been.
_cofactor_stids()now derives the set by ChEBI identity from the connected release (253 species) via the existingget_cofactor_species, with a small verified seed as the offline fallback. An empty fallback would silently stop excluding cofactors everywhere, so the seed is asserted non-empty.Measured: no behavioural change. Regenerated six pathways including every one where a species whose exclusion status changes is both produced and consumed (TP53, PTK6, RAF, PIP3, S_Phase, Mitotic_Prophase) — identical edge counts. The wrongly-listed species never arise as bridge or depletion candidates. Landed to stop the list rotting, not for accuracy.
2. The diagram's
linkswere never readdiagram_connectivityreads the layout JSON but used only itsedges. Mostlinksrestate set membership Neo4j already has — but not all.SMAD7:SMURF2andSMAD7:SMURF/NEDD4Lare both curated as Complexes with no containment between them (the generic holds aSMURF/NEDD4LDefinedSet where the specific holds SMURF2). The diagram joins them with anEntitySetAndMemberLink; the generated network had two unconnected nodes. Same shape for the EPH-ephrin oligomer complexes.diagram_set_member_pairsreturns those pairs member-first;_emit_diagram_set_member_edgesadds onepos/oredge per pair.One edge per stable-id pair, not the cartesian product. All-pairs turned two curated relationships into 48 edges in EPH-Ephrin alone — the blow-up that made the all-pairs silo bridge unusable. Capped to the best-connected occurrence on each side, it is 2.
Only
EntitySetAndMemberLinkis consumed.Interaction(245 in this catalog) and set-to-set links point at entities with no node here, so using them means adding nodes — a much larger change, issue #41.Adds 5 edges across the catalog: 3 in TGF-beta, 2 in EPH-Ephrin. Gated on
LNG_DIAGRAM_SET_MEMBER, default on.Expected impact
Close to none. Both are correctness fixes; I'd be surprised by measurable movement and will report the A/B either way.
Testing
5 new tests in
tests/test_diagram_set_member.pycovering link-class selection, the one-edge-per-pair cap, the missing-endpoint case, the derivation and its fallback, and that the six bad ids are absent from the seed.mypy clean, ruff clean, pytest 984 passed. The single failure (
test_main_edges_proportional_to_best_matches[Class_I_MHC…]) reproduces on a stashed tree and is unrelated.🤖 Generated with Claude Code