Skip to content

fix(ensemble): take exactly the requested fraction when subsampling - #1352

Open
aniruddhaadak80 wants to merge 2 commits into
PriorLabs:mainfrom
aniruddhaadak80:fix/subsample-fraction-off-by-one
Open

aniruddhaadak80 wants to merge 2 commits into
PriorLabs:mainfrom
aniruddhaadak80:fix/subsample-fraction-off-by-one

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

Issue

Fixes #1347

Motivation and Context

In preprocessing/ensemble.py the two branches that resolve subsample_samples disagree with each other and with the parameter's own docstring:

python if isinstance(subsample_samples, int): ... size = min(subsample_samples, n_samples) # exactly what was asked else: ... size = int(subsample_samples * n_samples) + 1 # one row more

The docstring says "If float, subsample that fraction of samples", so the +1 is simply wrong. Two observable defects follow:

  • Wrong size. SUBSAMPLE_SAMPLES=0.5 on 1000 rows subsamples 501 rows. The two spellings of the same request disagree: the float .5 and the int 500 build different model contexts.
  • Crash on a valid request. SUBSAMPLE_SAMPLES=0.01 on 100 rows with 3 classes resolves to size=2, which trips the guard subsample_size (2) must be >= number of classes (3). The +1 is what pushes a legal value into the illegal range - dropping it gives 1, and the only reason that would still be short is a dataset with more classes than rows sampled, which is the guard's real purpose.

No opt-in is needed to hit this: "stratified" is the resolved default for classifiers.

The fix takes the fraction exactly, truncating rather than rounding up, and keeps the floor at one row via max(1, ...) so a tiny fraction of a tiny dataset cannot resolve to an empty subsample.

Two existing tests encoded the old +1 arithmetic in their assertions and comments, and have been updated to the documented contract - one changes rac=0.2 to rac=0.25 to keep the same size=5 that its balanced-coverage assertion depends on.

Public API Changes

  • No Public API changes
  • Yes, Public API changes (Details below)

No signature changes. A float SUBSAMPLE_SAMPLES now yields exactly int(frac * n_samples) rows, matching its documentation and the int spelling.

How Has This Been Tested?

New tests in ests/test_preprocessing/test_ensemble.py:

  • est__get_subsample_indices_for_estimators__float_fraction_exact_size - asserts .5 of 1000 rows subsamples exactly 500, that the float and int spellings draw the same rows, and that .333 truncates to 333.
  • est__get_subsample_indices_for_estimators__float_fraction_below_class_count - the class-count case above.

The first fails on the unfixed source and passes with the fix:

`

unfixed source

FAILED ...::test__get_subsample_indices_for_estimators__float_fraction_exact_size
1 failed, 1 passed

with the fix

12 passed, 89 deselected
`

The full file gives 5 failed / 96 passed with the fix and 5 failed / 94 passed on a pristine ensemble.py - the same five failures either way, all differentiable_input and majority_downsample tests that fail without model weights, and the +2 difference is the new tests.

Worth noting: the second new test passes both before and after. It is kept as a guard on the crash rather than as a regression test for the fix.
uff check and
uff format --check are clean.

Checklist

  • The changes have been tested locally.
  • Documentation has been updated (if the public API or usage changes).
  • A changelog entry has been added (see changelog/README.md), or "no changelog needed" label requested.
  • The code follows the project's style guidelines.
  • I have considered the impact of these changes on the public API.

The float branch resolved to int(frac * n_samples) + 1, contradicting its
docstring and the sibling int branch's min(k, n). A 0.5 fraction of 1000 rows
subsampled 501 rows instead of 500, and 0.01 of 100 rows resolved to 2, which
tripped the guard requiring at least one row per class - reachable without any
opt-in because stratified is the resolved default for classifiers.
Copilot AI balanced review requested due to automatic review settings October 2, 2026 06:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions
github-actions Bot requested a review from anuragg1209 October 2, 2026 06:28

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit be7753d. Configure here.

Comment thread changelog/1352.fixed.md
@@ -0,0 +1 @@
Take exactly the requested fraction when SUBSAMPLE_SAMPLES is a float, instead of one row more, which also stopped small valid fractions from failing the class-count guard. No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog retells internal incident

Low Severity

The towncrier fragment narrates the old one-row overshoot and names the internal class-count guard. It also claims small valid fractions no longer fail that guard, but dropping the +1 makes more of those requests fail it.

Fix in Cursor Fix in Web

Triggered by learned rule: Towncrier fragments: user-facing change only; keep concise

Reviewed by Cursor Bugbot for commit be7753d. Configure here.

documents, and the equivalent int spelling (500) produced a different
context for the model. The float spelling must agree with the int spelling,
truncate rather than round up, and never resolve to an empty subsample.
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test docstring narrates prior behavior

Low Severity

The new test docstring recounts the previous int(frac * n_samples) + 1 arithmetic and why it was wrong, rather than stating only the current size contract.

Fix in Cursor Fix in Web

Triggered by learned rule: Docstrings describe current behavior only — no call sites or code history

Reviewed by Cursor Bugbot for commit be7753d. Configure here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A float SUBSAMPLE_SAMPLES takes one row too many, and a valid small fraction fails the class-count guard

2 participants