feat(prediction): bind group labels to a splitter for leak-free evaluation - #580
Merged
Merged
Conversation
…ation Protein datasets are full of dependent samples: several windows cut from one protein, near-identical homologues across a family. A plain split scatters them over train and test, so the model is scored partly on what it already saw. A scikit-learn group splitter prevents that, but it needs a `groups` array at split time, and every cross_val_* call in this package is made without one -- so a stock GroupKFold cannot be used at all. bind_groups closes that gap by binding the labels to the splitter, which is why AAPred.eval(cv=...) and ModelEvaluator.run(cv=...) take a group-aware splitter with no change to either. One adapter rather than three named classes: it wraps any splitter, so GroupKFold, StratifiedGroupKFold (keeps the class balance too), LeaveOneGroupOut (leave-one-protein-out, or leave-one-cluster-out over homology clusters) and GroupShuffleSplit are all covered by sklearn's own tested implementations, and the vocabulary lives in what is passed as `groups`. Two guards the issue asked for: - every fold is verified, so a splitter that ignores groups (KFold) is a ValueError naming the shared groups rather than a silent leak; allow_overlap=True permits it deliberately - an infeasible fold count is rejected at construction, with the requested folds and the group count in the message Once the folds are consumed, df_folds_ reports per-fold sample counts, group counts and class balance, so uneven group folds are inspectable rather than assumed. Measured on a seeded fixture of twelve proteins with six near-identical windows each: a random split reports 1.00 accuracy, the protein-grouped split 0.67. That contrast is the example notebook. The package performs the split and reports its properties; it runs no homology search or clustering itself. Addresses #478. Co-Authored-By: Claude Fable 5.1 <[email protected]>
The pyright ratchet caught 14 new diagnostics: every helper declared its required parameters as `=None`, so `None` propagated into every inference downstream (`self._cv.split` on `None`, `len(self._groups)` on `None`). Required parameters now carry no default, which is the rule the earlier burn-down established. No behaviour change: the call sites already passed every argument by keyword, and `_comp_pos_rate` keeps `labels=None` because None is a real value there (no labels -> NaN rate). Co-Authored-By: Claude Fable 5.1 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #580 +/- ##
==========================================
- Coverage 95.39% 95.39% -0.01%
==========================================
Files 221 222 +1
Lines 23303 23387 +84
Branches 4057 4073 +16
==========================================
+ Hits 22231 22309 +78
- Misses 627 631 +4
- Partials 445 447 +2
🚀 New features to boost your workflow:
|
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.
Protein datasets are full of dependent samples: several windows cut from one protein, near-identical homologues across a family. A plain split scatters them over train and test, so the model is scored partly on what it already saw.
A scikit-learn group splitter prevents that — but it needs a
groupsarray at split time, and everycross_val_*call in this package is made without one, so a stockGroupKFoldcannot be used at all.bind_groupscloses that gap by binding the labels to the splitter, which is whyAAPred.eval(cv=...)andModelEvaluator.run(cv=...)accept a group-aware splitter with no change to either — the issue's headline acceptance criterion.Why one adapter rather than three named classes
It wraps any splitter, so
GroupKFold,StratifiedGroupKFold(keeps the class balance too),LeaveOneGroupOut(leave-one-protein-out, or leave-one-cluster-out over homology clusters) andGroupShuffleSplitare all covered by sklearn's own tested implementations. The domain vocabulary lives in what is passed asgroups, which is also what makes the same call work for proteins, families and externally computed clusters. Two of the three names the issue proposed (LeaveOneProteinOut,LeaveOneClusterOut) would have been the same implementation.Two guards
KFold) raises aValueErrornaming the shared groups, instead of leaking silently.allow_overlap=Truepermits it deliberately, e.g. to reproduce an ungrouped baseline.Once the folds are consumed,
df_folds_reports per-fold sample counts, group counts and class balance, so the uneven folds a group splitter necessarily produces are inspectable rather than assumed.The leak, measured
On a seeded fixture of twelve proteins with six near-identical windows each, where the label is a property of the protein:
That contrast is the example notebook.
Verification
tests/unit/prediction_tests/test_bind_groups.py, including the no-overlap KPI over all folds, the infeasible-fold message, a hand-computedpos_rate_testgolden value, reproducibility of a seeded splitter, and theAAPred.evalend-to-end path.prediction_tests+api_tests+config_tests.##headings, which becomeCRITICAL: Unexpected section titlein the generated page that isinclude::d into the docstring. Existing example notebooks carry no headings; this one now matches.[Roberts17]added to the references (cross-validation for structured data).Addresses #478.
🤖 Generated with Claude Code