Skip to content

fix(inference): validate tuning holdout frac before inverting it for folds - #1350

Closed
aniruddhaadak80 wants to merge 3 commits into
PriorLabs:mainfrom
aniruddhaadak80:fix/tuning-holdout-frac-div
Closed

aniruddhaadak80 wants to merge 3 commits into
PriorLabs:mainfrom
aniruddhaadak80:fix/tuning-holdout-frac-div

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

Issue

Fixes #1345

Motivation and Context

inference_tuning.py derived the fold count from a pre-rounded holdout fraction:

python rounded_holdout_frac = round(holdout_frac, 2) n_folds = max(2, round(1 / rounded_holdout_frac))

uning_holdout_frac is typed Literal["auto"] | float, and TuningConfig.resolve passes an explicit float through verbatim, so nothing validates it before the division. Three consequences:
  • Crash. =0.004 rounds to .0, so 1 / 0.0 raises ZeroDivisionError out of it(). The same holds for anything in (0, 0.005).
  • Crash. =0.005 rounds to .01, giving
    _folds=100, and StratifiedKFold(n_splits=100) raises
    _splits=100 cannot be greater than the number of members in each class on any realistic-sized dataset.
  • Silent wrong value. =0.15 becomes 7 folds - a 14.3% holdout, not the 15% requested.

The rounding was there to avoid needing a large number of folds, but applying it before the inversion is what collapses small fractions to zero. The fix validates the fraction is finite and in (0, 1), rejects fractions that would need more than MAX_TUNING_FOLDS = 100 folds, and inverts the exact value instead of a rounded one.

One deliberate behaviour change worth flagging for review: a fraction below .01 now raises a ValueError where it previously either crashed or silently substituted a different holdout rate. I judged an explicit error better than silent substitution, but if maintainers prefer clamping, MAX_TUNING_FOLDS is the single knob to revisit.

Public API Changes

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

No signature changes. Invalid uning_holdout_frac values now raise a clear ValueError instead of ZeroDivisionError or a silently different holdout rate.

How Has This Been Tested?

New tests in ests/test_inference_tuning.py cover: the ZeroDivisionError range now raising ValueError; the
_folds=100 case; the exact inversion for fractional values; and a pin asserting the "auto" default path is unchanged.

`

before the fix, at the get_tuning_splits level that fit() calls

frac=0.004 n=200 task=regressor -> ZeroDivisionError: float division by zero
frac=0.004 n=200 task=classifier -> ZeroDivisionError: float division by zero
frac=0.005 n=100 task=classifier -> ValueError: n_splits=100 cannot be greater than ...
frac=0.15 n=200 task=regressor -> OK holdout=29 (7 folds, not 6.67)
`

The new tests fail on the unfixed source (9 failed, 5 passed, with ZeroDivisionError at inference_tuning.py:269 and six DID NOT RAISE ValueError) and pass with the fix:

$ python -m pytest tests/test_inference_tuning.py -q 96 passed in 406.42s (0:06:46)

The "auto" path is unchanged: get_default_tuning_holdout_frac only ever returns 0.1/0.2/0.3, and the old and new formulas were compared across every threshold plus 100k samples with identical fold counts (10/5/5/5/3).
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.

…folds

`tuning_holdout_frac` is passed through verbatim, so any user value was fed
straight into `round(holdout_frac, 2)` before `1 / rounded_holdout_frac`.
Pre-rounding to 2 decimals collapsed values below 0.005 to zero and inflated
values just above it, so `tuning_holdout_frac=0.004` raised ZeroDivisionError
out of `fit()` and `0.005` asked for 100 folds, which StratifiedKFold rejects on
most real datasets. Out-of-range and non-finite fracs were silently accepted.

Derive the fold count from the exact fraction instead, and reject fractions that
are outside (0, 1) or that would need more folds than we support.
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.

@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 0be1d69. Configure here.

Comment thread changelog/1350.fixed.md Outdated
Comment thread src/tabpfn/inference_tuning.py Outdated
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Thanks - both points taken. Fixed in a4e3dfd.

  • changelog/1350.fixed.md is rewritten to state the behaviour from the user's perspective, with no incident narrative.
  • The comment above the fold-count expression is removed; the docstring on _folds_for_holdout_frac already documents the range it accepts and the ValueError it raises, so the inline comment was redundant.

@klemens-floege

Copy link
Copy Markdown
Contributor

Hi @aniruddhaadak80, thanks for the time you've put into TabPFN. A few of your earlier fixes were useful and we merged them.We've since adopted contribution guidelines, and your open issues and PRs don't fit them. External contributors can have at most 3 open PRs, and you have 10. The AI section also asks that a human reviews every submission before it goes in, answers review questions personally, and only reports problems they actually ran into. Bulk-filed reports from automated bug-hunting are called out as not welcome. Oscar's question on #1317 never got an answer.So we're closing all of your open issues and PRs. We'll fix #1344 and #1355 ourselves and credit you in the commits.If you want to keep contributing, please open one focused PR at a time that you've reviewed yourself, and reply to reviewers in your own words. If you think we got something wrong, reply here.

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.

tuning_holdout_frac below 0.005 raises ZeroDivisionError; near 0.005 it yields an unusable fold count

3 participants