Make break_apart_entity's memo return what the function returns (#58) - #64
Merged
Conversation
The memo re-reads the rows the function wrote, but for a Complex it read the
wrong columns, so the cached answer was a different kind of thing from the
uncached one — and the caller could not tell.
A Complex returns get_broken_apart_ids(...) = the COMBINATION uids, one per
set-variant, each standing for the whole complex. Its rows are written by
get_uids_for_iterproduct_components, which stores that combination in `uid` and
puts the individual COMPONENTS in input_or_output_uid /
input_or_output_reactome_id. The memo returned the union of those two columns,
i.e. the complex's subunits.
The caller feeds the result into a per-reaction itertools.product as
ALTERNATIVES. So on its second and every later appearance, a Complex was
modelled as "any one of my subunits". CCNA + CDK2 -> CCNA:CDK2 was emitted as
three virtual reactions whose outputs are single free proteins — "CDK2 alone is
a complete output of complex formation" — which the module docstring explicitly
forbids ("Complex = a bound species... treated atomically").
An EntitySet returns its MEMBERS, and its provenance rows do put the members in
those same two columns, so the memo was already correct there. The fix branches
on the label rather than rewriting both paths.
Measured
--------
Idempotence, R-HSA-69242: entities returning something different on
re-decomposition went from 11 of 48 to 0.
DeltaSignal A/B — both arms on DeltaSignal main, Reactome Release97, the ten
MP-BioPath evaluation pathways, experimental ground truth, differing only in
the memo:
control 627 scored 452 correct acc 0.7209 macro-F1 0.664946
memo fixed 627 scored 452 correct acc 0.7209 macro-F1 0.664946
NEUTRAL, and verified to be a real comparison rather than an accidental no-op:
only 3 of 43 network hashes are shared between the arms, and 221 of 847 raw
predicted values change, along with 389 gene-uuid counts and 22 convergence
flags — but not one classification. Convergence improves slightly
(non-converged 155 -> 149) for +0.8% solver iterations.
That is the expected shape for this fix. The duplicated virtual reactions were
byte-identical copies computing the same value from the same nodes, so removing
them changes magnitudes and iteration counts but not answers. The case for
landing it is correctness, plus the downstream consequences: it is the root
cause of the diagram_bridge blow-up (#61), of spurious MAX_VARIANTS truncation
(which silently bundles components into one opaque node), and of catalyst-edge
multiplication. Networks shrink accordingly — Mitotic G1 580 -> 493 virtual
reactions; catalog-wide 53,782 -> 39,848.
No flag. The default-OFF convention exists for modelling choices whose
direction is unknown; this is a function disagreeing with its own memo, with
zero prediction changes. Contrast LNG_SET_MEMBERS_OR, which is faithful but
cost 51 of 223 cases and correctly stays off.
Testing
-------
The lockdown test extends TestCrossCallStability, which asserted stability for
EntitySets only — the case the memo got right, which is why this survived. The
new test is confirmed to FAIL with the fix reverted.
Suite: 924 passed vs 923 on main, with the same 7 pre-existing failures (they
validate against the stale checked-in v96 output/ tree). Validator: 11/11 on
all three benchmark-eligible pathways on the regenerated networks.
Recorded as specs/002-break-apart-memo. Closes #58.
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
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.
The memo re-reads the rows the function wrote, but for a Complex it reads the wrong columns — so the cached answer is a different kind of thing from the uncached one, and the caller cannot tell.
EntitySetinput_or_output_*— where the provenance writer puts membersComplexinput_or_output_*— where the iterproduct writer puts componentsThe caller feeds the result into a per-reaction
itertools.productas alternatives, so on its second and every later appearance a Complex was modelled as "any one of my subunits":"CDK2 alone is a complete output of complex formation" — which the module docstring explicitly forbids. The fix branches on the label rather than rewriting the half that already worked.
Measured
Idempotence on R-HSA-69242: entities returning something different on re-decomposition went 11 of 48 → 0.
DeltaSignal A/B — both arms on DeltaSignal
main, Release97, the ten evaluation pathways, experimental ground truth, differing only in the memo:NEUTRAL — and verified to be a real comparison, not an accidental no-op. Only 3 of 43 network hashes are shared between arms, and the networks change substantially:
predicted_uigene_uuid_countconvergedConvergence improves slightly (non-converged 155 → 149) for +0.8% solver iterations.
That's the expected shape: the duplicated VRs were byte-identical copies computing the same value from the same nodes, so removing them changes magnitudes and iteration counts but not answers.
Why land it
Correctness, plus the downstream consequences — it is the root cause of the
diagram_bridgeblow-up (#61), of spuriousMAX_VARIANTStruncation (which silently bundles components into one opaque node), and of catalyst-edge multiplication. Networks shrink accordingly: Mitotic G1 580 → 493 VRs, catalog-wide 53,782 → 39,848.No flag. The default-OFF convention exists for modelling choices whose direction is unknown. This is a function disagreeing with its own memo, with zero prediction changes. Contrast
LNG_SET_MEMBERS_OR— faithful, but cost 51 of 223 cases and correctly stays off.Testing
The lockdown test extends
TestCrossCallStability, which asserted stability for EntitySets only — the case the memo got right, which is why this survived. Confirmed to fail with the fix reverted.Suite: 924 passed vs 923 on main, same 7 pre-existing failures (they validate against the stale checked-in v96
output/tree). Validator 11/11 on all three benchmark-eligible pathways.Recorded as
specs/002-break-apart-memo. Closes #58.🤖 Generated with Claude Code