Make the EntitySet coverage guard reachable, and give regulator propagation a floor - #71
Merged
Merged
Conversation
…gation a floor An adversarial review of PR #57 found that the guard it added could never fire. This is the same over-broad-excuse mistake, one layer down from where I fixed it, and it is the third time in this review I have made it. _uncovered() applied the decomposed-Complex excuse BEFORE the EntitySet analysis. A split EntitySet is itself a decomposition parent — its stId sits in decomposed_uid_mapping.reactome_id — so every set was excused as "present" up front, `direct` came back empty, the function took its early return, and _set_leaf_members() was never called at all. The `partial` branch was dead code across all 10 catalogs. Demonstrated: deleting ALL SEVEN members of R-HSA-1445138 from R-HSA-69620, leaving that set with zero representation in the network, still reported ✅ PASS: Entity Coverage ℹ️ 9 EntitySets represented by their members ✓ Tests passed: 11/11 The docstring promised the exact opposite — "a set the generator split only halfway is a real defect, so it is reported separately rather than excused". Ordering is now: analyse the set first, and apply the decomposed excuse to the MEMBERS rather than the set. That distinction matters and I got it wrong on the first attempt: excusing nothing made six healthy pathways fail, because a member can legitimately be a decomposed Complex represented by its components (R-HSA-8945704 has 72 rows in the decomposed mapping). The set is never excused for being a decomposition parent; a member is excused for being a decomposed Complex. validate_regulator_propagation had zero result.fail() calls — structurally incapable of failing, the identical defect PR #57's own commit message announces fixing in validate_edge_counts one method away. Deleting every regulator edge gave "PASS: Regulator Propagation" and 11/11. Expansion means an exact count assertion is impossible, so it asserts the floor: a role Neo4j records must not vanish entirely. The resolver's "Pathway not found" SystemExit was unreachable — both branches assign one of the two fields unconditionally, so the conjunction was never true and --pathway-id R-HSA-99999999 produced the malformed glob "*_R-HSA-R-HSA-99999999/" before failing later with a misleading message. It now tests the field the lookup was meant to fill. Verified both directions, which is the part that was missing before: all 7 members of a set deleted -> FAIL, 10/11 1 of 7 members deleted -> FAIL, names R-HSA-194364, 10/11 all regulator edges deleted -> FAIL, 9/11 nonexistent pathway id -> clean error, both id forms 10 evaluation pathways, clean -> 9x 11/11, WNT 10/11 (issue #59, real) Suite 952 passed, ruff clean. 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.
An adversarial review of PR #57 found that the guard it added could never fire. Same over-broad-excuse mistake, one layer down from where I fixed it — third time this review.
The dead guard
_uncovered()applied the decomposed-Complex excuse before the EntitySet analysis. A split EntitySet is a decomposition parent — its stId is indecomposed_uid_mapping.reactome_id— so every set was excused as "present" up front,directcame back empty, the early return fired, and_set_leaf_members()was never called. Thepartialbranch was dead across all 10 catalogs.Deleting all seven members of
R-HSA-1445138, leaving the set with zero representation:The docstring promised the opposite: "a set the generator split only halfway is a real defect, so it is reported separately rather than excused."
The fix, and the mistake I made fixing it
Analyse the set first; apply the decomposed excuse to the members, not the set.
That distinction matters — my first attempt excused nothing and six healthy pathways failed, because a member can legitimately be a decomposed Complex represented by its components (
R-HSA-8945704has 72 rows in the decomposed mapping). A set is never excused for being a decomposition parent; a member is excused for being a decomposed Complex.Two more dead guards in the same file
validate_regulator_propagationhad zeroresult.fail()calls — the identical defect PR #57's own commit message announces fixing invalidate_edge_counts, one method away. Deleting every regulator edge passed 11/11. Expansion makes an exact count assertion impossible, so it now asserts the floor: a role Neo4j records must not vanish entirely.The resolver's "Pathway not found"
SystemExitwas unreachable — both branches assign unconditionally, so--pathway-id R-HSA-99999999produced the malformed glob*_R-HSA-R-HSA-99999999/.Verified both directions
R-HSA-194364Suite 952 passed, ruff clean.
🤖 Generated with Claude Code