Repository navigation
Class balancing is ignored in cross-validation - #144
Open
ellispatrick wants to merge 3 commits into
Open
ellispatrick wants to merge 3 commits into
ellispatrick wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Balancing before nested-CV splitting can alter validation sets and leak upsampled observations across folds.
2 open findings
What changed in this PR
Fixes class balancing during cross-validation while preserving default behavior.
Changes:
- Applies configured balancing to cross-validation training sets.
- Adds a downsampling regression test.
| File | Description |
|---|---|
R/runTest.R |
Enables balancing for cross-validation iterations. |
tests/testthat/test-core.R |
Tests downsampling of training sets. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # Rebalance the class sizes of the training samples by either downsampling or upsampling, also when called by | ||
| # runTests. Within runTests and with no balancing, the training samples are left as they are (and in the same order). | ||
| if((is.null(.iteration) || modellingParams@balancing != "none") && !is(outcomeTrain, "Surv")) |
Comment on lines
+231
to
+245
| trainSizes <- function(balancing) { | ||
| sizes <- integer() | ||
| counting <- function(measurementsTrain, classesTrain, ...) { | ||
| sizes <<- c(sizes, length(classesTrain)) | ||
| ClassifyR:::DLDAtrainInterface(measurementsTrain, classesTrain, ...) | ||
| } | ||
| params <- ModellingParams(balancing = balancing, selectParams = NULL, trainParams = TrainParams(counting), | ||
| predictParams = PredictParams("DLDA")) | ||
| set.seed(1) | ||
| runTests(measurements, classes, CrossValParams(permutations = 1, folds = 4, parallelParams = BiocParallel::SerialParam()), params) | ||
| sizes | ||
| } | ||
| none <- trainSizes("none"); down <- trainSizes("downsample") | ||
| expect_true(all(down < none)) | ||
| expect_true(all(down %% 2 == 0)) # Equal numbers of each class. |
…, not only in direct runTest calls runTest rebalanced the training classes only inside its `if(is.null(.iteration))` branch, which runs when a user calls runTest directly. Within runTests, balancing = "downsample" or "upsample" was silently ignored. Rebalancing now also happens within runTests when balancing is not "none"; with "none" the training samples are left as they are, so default results are unchanged (identical predictions checked for DLDA, random forest and SVM). crossValidate still uses balancing = "none". Adds a test that counts the training samples given to the classifier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TfsjwkbDbHkX4kJZNe4ER4
ellispatrick
force-pushed
the
review/17-new-classifiers
branch
from
October 14, 2026 00:20
606c8da to
df94aa4
Compare
ellispatrick
force-pushed
the
review/18-balancing
branch
from
October 14, 2026 00:20
00f20cc to
6d1426f
Compare
…ning samples before rebalancing runTest rebalanced the training classes before feature selection and tuning, so .innerCVperformance built its inner folds from the rebalanced samples. With upsampling, copies of one sample could be in both an inner training and test set (and the inner predictions with duplicated sample names failed to assemble, so upsampling with nested CV gave errors); with downsampling, the inner test sets were downsampled too. runTest now keeps the samples before rebalancing and passes them to the selection and training stages, whose nested cross-validation makes its folds from them, using the features being evaluated; runTest rebalances each inner training set. Ranking, the final fit and resubstitution tuning use the rebalanced training samples, as before. This applies to runTests and direct runTest calls. Results with balancing = "none", and with balancing but no nested-CV tuning, are identical (regression scenarios plus runTests/runTest scenarios with each balancing and tuneMode). Results of balancing with nested-CV tuning change. The balancing test now checks the class counts given to the classifier, and a new test checks that no sample is in both an inner training and test set when upsampling with nested-CV tuning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y1PuRkfDQ3ApzuNoe5ovxW
The result's sample names and actual outcomes were taken from the rebalanced training set, so upsampling repeated sample names (calcCVperformance then failed with "factor level is duplicated") and downsampling dropped training samples. They now come from the training samples before rebalancing. Results without balancing are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y1PuRkfDQ3ApzuNoe5ovxW
This branch has not been deployed
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.
Problem.
ModellingParams(balancing = "downsample")(or"upsample") has no effect inrunTests.runTestcalls.rebalanceTrainingClassesonly insideif(is.null(.iteration)), the branch for a user callingrunTestdirectly, so within cross-validation the training classes are never rebalanced. This is also the case ondevel.crossValidatehard-codesbalancing = "none"and is unaffected.Reproduce (imbalanced data, DLDA): predictions with
balancing = "none"and"downsample"are identical (66 vs 14 calls in both).Change. Rebalance within
runTestswheneverbalancingis not"none". With"none"the training samples are left as they are, so default results are unchanged; I checked identical predictions for DLDA, random forest and SVM fromcrossValidate. DirectrunTestcalls behave as before.Test.
test-core.R: counts the training samples given to the classifier; with downsampling every training set is smaller and has equal class sizes.Stacked on #143.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TfsjwkbDbHkX4kJZNe4ER4