From 28176d8f8a299dca875797cdc4af0357aaf7357d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Thu, 10 Sep 2026 12:20:50 -0400 Subject: [PATCH] Make the EntitySet coverage guard reachable, and give regulator propagation a floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- scripts/validate_logic_network.py | 92 ++++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/scripts/validate_logic_network.py b/scripts/validate_logic_network.py index 46938e3..68d7a73 100755 --- a/scripts/validate_logic_network.py +++ b/scripts/validate_logic_network.py @@ -119,8 +119,16 @@ def _resolve_pathway_ids(self): ).evaluate() self.pathway_dbid = dbid self.pathway_stid = row - if self.pathway_stid is None and self.pathway_dbid is None: - raise SystemExit(f"Pathway {raw!r} not found in Neo4j.") + # Unreachable as originally written: both branches above assign one of + # the two fields unconditionally, so the conjunction was never true and + # a nonexistent id sailed through to produce the malformed glob + # "*_R-HSA-R-HSA-99999999/". Test the field the LOOKUP was meant to + # fill, which is the one that is None when the pathway does not exist. + if self.pathway_dbid is None or self.pathway_stid is None: + raise SystemExit( + f"Pathway {raw!r} not found in Neo4j (Reactome release " + f"{os.getenv('LNG_REACTOME_RELEASE', 'current')})." + ) # Directory names carry the stable id, Cypher prefers it too. self.pathway_id = self.pathway_dbid if self.pathway_dbid is not None else raw @@ -388,21 +396,33 @@ def validate_regulator_propagation(self) -> ValidationResult: result.add_info(f"Neo4j: {neo4j_catalyst_count} reactions with catalysts") result.add_info(f"Logic network: {logic_catalyst_reactions} virtual reactions with catalysts") - # Note: Logic network may have more because of EntitySet decomposition - if logic_pos_reactions >= neo4j_pos_count: - result.add_info("Positive regulators: ✓ (may be duplicated for virtual reactions)") - else: - result.warn(f"Missing positive regulators: expected >={neo4j_pos_count}, got {logic_pos_reactions}") - - if logic_neg_reactions >= neo4j_neg_count: - result.add_info("Negative regulators: ✓ (may be duplicated for virtual reactions)") - else: - result.warn(f"Missing negative regulators: expected >={neo4j_neg_count}, got {logic_neg_reactions}") - - if logic_catalyst_reactions >= neo4j_catalyst_count: - result.add_info("Catalysts: ✓ (may be duplicated for virtual reactions)") - else: - result.warn(f"Missing catalysts: expected >={neo4j_catalyst_count}, got {logic_catalyst_reactions}") + # Expansion means the network legitimately has MORE regulated virtual + # reactions than Neo4j has regulated reactions, so a shortfall cannot + # be an exact assertion. But a role Neo4j records must not vanish + # entirely: that is a floor, and it is checkable. + # + # Every branch below used to be warn() only, so this check could not + # fail — deleting every regulator edge in the network still gave + # "PASS: Regulator Propagation" and 11/11. That is the same defect + # fixed in validate_edge_counts, one method away, and missed here. + for label, expected_count, emitted in ( + ("positive regulator", neo4j_pos_count, logic_pos_reactions), + ("negative regulator", neo4j_neg_count, logic_neg_reactions), + ("catalyst", neo4j_catalyst_count, logic_catalyst_reactions), + ): + if emitted >= expected_count: + result.add_info( + f"{label.capitalize()}s: ✓ (may be duplicated for virtual reactions)" + ) + elif emitted == 0 and expected_count > 0: + result.fail( + f"Neo4j has {expected_count} reactions with a {label} but the " + f"logic network has none at all" + ) + else: + result.warn( + f"Missing {label}s: expected >={expected_count}, got {emitted}" + ) return result @@ -624,23 +644,45 @@ def _uncovered(self, expected: set, present: set, id_property: str): members that did *not* survive: a set the generator split only halfway is a real defect, so it is reported separately rather than excused. """ - # A decomposed Complex is represented by its components, not by its own - # id, so count it as present rather than missing. - present = present | self._decomposed_ids() - + # ORDER MATTERS. The decomposed-Complex excuse must be applied AFTER + # the EntitySet analysis, never before it. + # + # A split EntitySet is itself a decomposition parent — its stId sits in + # decomposed_uid_mapping.reactome_id — so unioning _decomposed_ids() + # into `present` up front excused every set before its members were + # ever examined, took the early return, and never called + # _set_leaf_members at all. The `partial` branch below was therefore + # unreachable: deleting ALL seven members of R-HSA-1445138 still + # reported "9 EntitySets represented by their members" and 11/11. + # + # That is the same over-broad excuse this helper was rewritten to + # remove, one layer down: the earlier fix restored sensitivity for + # plain entities and left sets fully masked. direct = expected - present if not direct: return set(), {} members = self._set_leaf_members(direct, id_property) + decomposed = self._decomposed_ids() missing, partial = set(), {} for entity_id in direct: leaves = members.get(entity_id) - if not leaves: - # Not a set (or a set with no resolvable leaves) — genuinely absent. + if leaves: + # A set is covered only when its members actually survived. + # Being a decomposition parent does not excuse the SET — that + # was the masking bug — but it does excuse a MEMBER: a member + # that is itself a decomposed Complex is represented by its + # components rather than by its own id, so it is present in + # the only sense available to it. + absent = leaves - present - decomposed + if absent: + partial[entity_id] = absent + elif entity_id in decomposed: + # A decomposed Complex is represented by its components rather + # than by its own id. + continue + else: missing.add(entity_id) - elif not leaves <= present: - partial[entity_id] = leaves - present return missing, partial def validate_entity_coverage(self) -> ValidationResult: