Handle small-sample cross-fitting folds gracefully - #152
Merged
Merged
Conversation
- Skip folds (or fold x stratum cells) with nothing to predict instead of calling the model on an empty array, which raised an opaque sklearn 'Found array with 0 sample(s)' error. - Check up front that every fold x stratum cell has target-arm training data and that every stratum contains the target arm; raise a ValueError that names the fold/stratum and suggests reducing folds or merging strata instead of an IndexError. - Multi-task: when a cell's training labels are constant, predict that constant rather than reusing a model fit on another fold/stratum (or failing with AttributeError when none exists yet). Follow-up to the JOSS review minor point on empty folds (openjournals/joss-reviews#11047). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LjhUSYBaHyrPdnQ7z13FKv
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.
Follow-up to the JOSS review's minor point on empty folds (openjournals/joss-reviews#11047):
#150 added a check for one case only: a fold leaving no target-arm training data. Most small-sample failures took other paths and still surfaced as opaque errors.
Changes (
dte_adj/stratified.py,dte_adj/util.py)model.predict_probaon an empty array. sklearn then raisedFound array with 0 sample(s). Such a cell has nothing to predict, so it is now skipped. This was the most common failure, and those runs now simply succeed.IndexError: index 0 is out of bounds. The error now names the fold and stratum and suggests reducingfoldsor merging small strata.AttributeError: ... has no attribute 'model'. It now predicts the constant label, as the single-task path already does.Effect (200 random seeds each,
LogisticRegression, single-task)IndexError/ sklearn)With multi-task (
LinearRegression), 3 strata, n=20 and 5 folds, 24 runs used to fail withAttributeErrorand 134 with the informative error. After the change, all 146 failures are informative. Failures can still happen at sizes this small. The difference is that each one now tells the user what went wrong and what to do.Testing
TestSmallSampleFolds, covering an empty prediction fold, a stratum without training data, a stratum without the target arm, and multi-task constant labels. All four fail onmainand pass with this change.ruff check .is clean.