Rename Outcome -> EffectCandidate; document its purpose - #380
Closed
iskandr wants to merge 4 commits into
Closed
Conversation
…nt fields The class was clearly named to its users but the docs framed ``MultiOutcomeEffect.candidates`` as "back-compat" while it's actively the canonical internal data store. Three cleanups here: 1. **Rename ``Outcome`` -> ``EffectCandidate``**, ``outcomes_from_candidates`` -> ``candidates_from_effects``. The wrapper isn't an "outcome"; it's a candidate effect with per-context provenance. 2. **Drop ``EffectCandidate.description``** — pure passthrough to ``effect.short_description``. Producers wanting a richer narrative set it on the underlying Effect, not the wrapper. 3. **Rewrite docstrings** to explain *why* the wrapper exists (the same Effect appears in multiple contexts with different metadata — e.g. a SpliceOutcomeSet candidate re-surfaced by the SV annotator with a different source tag). The wrapper is the cheapest way to keep Effects shared while letting labels diverge. ``candidates`` and ``outcomes`` are documented as two complementary first-class accessors, not legacy vs new. 4. **Scrub aspirational "isovar" / "exacto" example tags** from varcode docstrings. Those integrations don't exist yet — pointing at them as canonical examples implied an integration that hasn't shipped. Hard rename, no deprecation alias. The class was exported as ``varcode.Outcome``; update imports to ``varcode.EffectCandidate``. 1187 tests pass, 2 pre-existing skips. CHANGELOG entry under Breaking.
- Rename varcode/outcomes.py -> varcode/effect_candidates.py and tests/test_outcomes.py -> tests/test_effect_candidates.py to match the class rename. - Rename make_rna_outcome -> make_rna_candidate (was the only public surface still using the old terminology). - Rename test methods test_outcome_* -> test_effect_candidate_*. - Add test pinning that EffectCandidate(description=...) raises TypeError so the removed kwarg can't sneak back via copy/paste. - CHANGELOG entry now links #380 directly instead of using a placeholder reference. The internal _with_extra_outcomes hook and _extra_outcomes storage slot keep their names — they hold EffectCandidate (i.e. outcomes- shaped) entries, so the existing names accurately describe the data being stored. 1188 pass, 2 pre-existing skips.
The class was named EffectCandidate but lived in eff.outcomes — an inversion that read as messy. Realign so the class name matches the accessor it returns from. Naming is now consistent throughout: * "candidates" everywhere -> raw MutationEffect objects (subclass internal storage, the .candidates accessor) * "outcomes" everywhere -> the wrapped form (EffectOutcome class, the .outcomes accessor, _extra_outcomes slot, observed_outcomes protocol method) Renames: * EffectCandidate -> EffectOutcome * candidates_from_effects -> outcomes_from_effects * varcode.effect_candidates -> varcode.effect_outcomes (module) * tests/test_effect_candidates.py -> tests/test_effect_outcomes.py * tests/test_effect_candidate_* -> tests/test_effect_outcome_* The make_rna_candidate -> make_rna_outcome change from the prior commit is also reverted in this pass (it's an outcome producer; the "outcome" terminology is correct after this realignment). 1188 tests pass, 2 pre-existing skips.
You disliked "outcome" naming on the wrapper. Reverting to EffectCandidate. The collapse to a single accessor was tried but broke ~30 splice/RNA tests because SpliceOutcomeSet.candidates has a deep history of returning SpliceCandidate (a different type), and serialization round-trips through that name; collapsing forces either widespread test migration or back-compat shims that defeat the cleanup. Final state on this PR: - Class: EffectCandidate (has .effect, .source, .probability, .evidence) - MultiOutcomeEffect.candidates -> tuple[MutationEffect, ...] (raw) - MultiOutcomeEffect.outcomes -> tuple[EffectCandidate, ...] (wrapped) - The accessor/type mismatch is documented as two complementary first-class accessors. Not as clean as fully aligned, but real. 1188 pass.
5 tasks
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.
Stacked on #379 (generic-phasing-api). Merge that first; this PR will rebase to main.
Summary
Three cleanups on the multi-outcome machinery, addressing the "is this AI slop?" diagnosis:
Rename
Outcome→EffectCandidate. Same withoutcomes_from_candidates→candidates_from_effects. The class isn't an outcome; it's a candidate effect with per-context provenance.Drop
EffectCandidate.description. The field was a pure passthrough toeffect.short_description. Producers wanting a richer narrative attach it to the underlying Effect, not the wrapper.Document why the wrapper exists. New module + class docstring in
varcode/outcomes.py(and corrected docstring onMultiOutcomeEffect) explain the actual rationale: the sameMutationEffectinstance gets surfaced from multiple multi-outcome contexts with different per-context metadata. Concrete example: a splice candidate built by varcode's splice classifier is re-used by the SV annotator with a differentsourcetag andsv_typeevidence. Putting metadata on the wrapper instead of the Effect lets the Effect stay shared while the labels diverge.Previously the docstring framed
candidatesas "kept for back-compat with 2.x" — misleading, since it's still the canonical internal data store. Now bothcandidates(raw Effects) andoutcomes(wrapped) are documented as complementary first-class accessors with distinct, legitimate use cases.Scrub aspirational "isovar" / "exacto" example tags from varcode docstrings. Those integrations don't exist yet; treating them as canonical examples in varcode's docs created a fake dependency.
What changed under the hood
Outcomeclass →EffectCandidateclass. Module path unchanged (varcode.outcomes).Outcome.descriptionfield removed;make_rna_outcome(..., description=)parameter removed.MultiOutcomeEffectdocstring rewritten — no more "back-compat" framing.varcode/outcomes.py.What didn't change
MultiOutcomeEffect.candidatesvsMultiOutcomeEffect.outcomesdual access. Looking closer, the two serve distinct purposes (candidates= raw Effects for plain iteration;outcomes= wrapped for per-context metadata) — collapsing them would force either copies everywhere or verbose access at every consumer. Documented the distinction sharply instead.Test plan
pytest tests/— 1187 pass, 2 pre-existing skips.test_outcomes_from_candidates_tags_source→test_candidates_from_effects_tags_source; two tests that readEffectCandidate.descriptiondeleted/trimmed.Outcomeimport ordescription=kwarg updated.Files touched
19 files, +228 / -243 LOC.
References
Outcomeintroduction (the docstring's "Unified MultiOutcomeEffect / OutcomeSet generalization across splice, readthrough, germline, and RNA evidence #299 harmonized interface" framing is what this PR cleans up).