Skip to content

fix(optimization): support hold-out CV in Optuna tuning path - #58

Open
thomasATbayer wants to merge 6 commits into
mainfrom
enable_hold_out_Set_usage_for_optuna
Open

fix(optimization): support hold-out CV in Optuna tuning path#58
thomasATbayer wants to merge 6 commits into
mainfrom
enable_hold_out_Set_usage_for_optuna

Conversation

@thomasATbayer

Copy link
Copy Markdown
Collaborator

Branch: enable_hold_out_set_usage

Overview

This branch isolates the hold-out validation fix from the broader sampler discussion.

MotherML supports Optuna early stopping via TerminatorCallback, but that callback requires at least two cross-validation folds to compute regret-based stopping criteria. In hold-out validation (n_splits < 2), attaching TerminatorCallback is invalid.

Additionally, Optuna's report_cross_validation_scores requires more than one fold score. With hold-out validation, only one score exists, so reporting that score also raises an error.

This branch fixes both hold-out failure paths.

Fixed - src/mother/optimization/core.py

Hold-out guard in get_callbacks

get_callbacks now accepts an optional cross_validation argument and checks the number of splits before attaching TerminatorCallback.

If n_splits < 2, it now:

  • logs a warning
  • returns None
  • avoids attaching TerminatorCallback

This makes hold-out tuning safe and keeps early stopping enabled only for valid multi-fold CV setups.

# Before
def get_callbacks(self): ...

# After
def get_callbacks(self, cross_validation=None): ...
# returns None early with warning if n_splits < 2

Guarded cross-validation score reporting in objective

report_cross_validation_scores is now called only when there are at least two non-NaN fold scores.

For hold-out validation (single score), tuning now skips reporting to the terminator API and continues normally.

if len(cv_score_not_na) > 1:
	report_cross_validation_scores(trial, list(cv_score_not_na))

Tests

Added / updated coverage in test/unit/test_model_tuner.py

  • hold-out CV returns None from get_callbacks
  • hold-out CV emits a warning
  • proper multi-fold CV does not trigger the hold-out early return
  • calling get_callbacks() without a CV object still works
  • MotherTuner.optimize(...) works end-to-end with hold-out (PredefinedSplit) and early_stopping_optuna=True

Why this should be merged independently

  • It is a correctness fix.
  • It prevents invalid early-stopping configuration for hold-out tuning.
  • It prevents objective-time failures from single-fold score reporting.
  • It is small, isolated, and easy to review.
  • It does not depend on the broader GPSampler / TPESampler default-sampler decision.

Suggested commit message

fix(optimization): support hold-out cv in early-stopping tuning path

Suggested PR wording

This PR fixes hold-out validation in the Optuna tuning path. It addresses two related issues: (1) TerminatorCallback was being attached even when CV had fewer than two folds, and (2) report_cross_validation_scores was being called with a single hold-out score, which raises an error in Optuna. The fix now disables terminator callbacks for hold-out setups and conditionally reports CV scores only when at least two fold scores are available. Tests cover callback behavior for hold-out and multi-fold CV, no-CV backward compatibility, and an end-to-end hold-out optimize run.

disable TerminatorCallback when CV has fewer than 2 splits
avoid calling report_cross_validation_scores for single-fold hold-out runs
pass cross_validation into get_callbacks from optimize
add callback behavior tests for hold-out, multi-fold CV, and no-CV path
add end-to-end optimize test with PredefinedSplit hold-out setup
If you want a slightly shorter subject line:
Copilot AI review requested due to automatic review settings July 23, 2026 13:09
@thomasATbayer thomasATbayer linked an issue Jul 23, 2026 that may be closed by this pull request

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.

Pull request overview

This PR fixes Optuna tuning behavior when using hold-out validation (single split), where Optuna’s terminator integration is invalid: it now avoids attaching TerminatorCallback for < 2 splits and only reports fold scores to Optuna’s terminator API when at least two non-NaN fold scores are present.

Changes:

  • Added a CV split-count guard to disable TerminatorCallback for hold-out setups in MotherTuner.get_callbacks(...).
  • Made report_cross_validation_scores(...) conditional on having more than one non-NaN fold score.
  • Added unit tests covering hold-out behavior, multi-fold behavior, and an end-to-end optimize run with hold-out CV.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/mother/optimization/core.py Prevents invalid Optuna terminator usage for hold-out CV and avoids single-score terminator reporting errors.
test/unit/test_model_tuner.py Adds regression tests for hold-out CV early-stopping behavior and end-to-end tuning.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/unit/test_model_tuner.py
Comment thread src/mother/optimization/core.py
… dependency

skip TestGetCallbacks when torch is unavailable (mother[torch] not installed)
keep default dev/test runs independent of optional extras
update get_callbacks docstring to include hold-out (n_splits < 2) None-return behavior
Copilot AI review requested due to automatic review settings July 23, 2026 13:43

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

configure TerminatorCallback with Terminator(min_n_trials=40)
keep hold-out guard: disable terminator callback when CV has fewer than 2 splits
keep objective behavior for hold-out by skipping report_cross_validation_scores for single-score CV
preserve existing tuner callback and sampler test coverage
Copilot AI review requested due to automatic review settings July 23, 2026 13:53

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment thread src/mother/optimization/core.py
Comment thread src/mother/optimization/core.py Outdated
Comment thread test/unit/test_model_tuner.py Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 13:56

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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/mother/optimization/core.py:195

  • get_callbacks() now hard-codes Terminator(min_n_trials=40), which changes early-stopping behavior vs. the prior TerminatorCallback() default. In particular, if a user configures n_trials_optuna < 40, the terminator will never become eligible and early stopping is effectively disabled even for valid multi-fold CV runs. Consider either using TerminatorCallback() defaults again, or making min_n_trials configurable / derived from self.n_trials_optuna.
                if cross_validation is not None and cross_validation.get_n_splits() < 2:
                    module_logger.warning(
                        "Optuna early termination requires at least 2 CV splits; disabling callback for hold-out setup"
                    )
                    return None
                callbacks = [TerminatorCallback(terminator=Terminator(min_n_trials=40))]
        return callbacks

set n_startup_trials default to 20
require at least 40 completed trials before terminator can stop (min_n_trials=40)
keep hold-out safeguards so single-split CV still returns objective performance safely

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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/mother/optimization/core.py:194

  • get_callbacks() now hard-codes Terminator(min_n_trials=40), which changes early-stopping behavior globally and can effectively disable early stopping when n_trials_optuna < 40. If this PR is only meant to make hold-out safe, consider keeping the previous default TerminatorCallback() behavior (or make the terminator configuration a user-facing parameter).
                callbacks = [TerminatorCallback(terminator=Terminator(min_n_trials=40))]

Comment thread src/mother/optimization/core.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 23, 2026 15:04

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.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/mother/optimization/core.py:142

  • n_startup_trials default is changed to 20 here, but MotherTunerConfig still defaults to 12 and the class docstring still references 12. This introduces inconsistent defaults depending on how the tuner is instantiated and is outside the stated hold-out CV fix scope. Consider keeping the prior default (12) or aligning defaults across config + docs in a separate change.
        n_trials_optuna: int = 100,
        n_threads_optuna: int = 1,
        n_startup_trials: int = 20,
        seed: int = 42,
        **kwargs,

src/mother/optimization/core.py:195

  • This switches the Optuna terminator callback from the library default to a custom Terminator(min_n_trials=40). That’s a behavior change (and introduces an unexplained magic number) that may prevent early stopping for runs with <40 trials, and it’s not required for the hold-out CV guard. If no custom terminator is needed, keep the default callback behavior.
                if cross_validation is not None and cross_validation.get_n_splits() < 2:
                    module_logger.warning(
                        "Optuna early termination requires at least 2 CV splits; disabling callback for hold-out setup"
                    )
                    return None
                callbacks = [TerminatorCallback(terminator=Terminator(min_n_trials=40))]
        return callbacks

src/mother/optimization/core.py:20

  • If you revert to the default TerminatorCallback() behavior, the Terminator import becomes unnecessary. Keeping imports minimal avoids unused-import lint failures and makes the early-stopping integration easier to follow.
from optuna.terminator import (
    Terminator,
    TerminatorCallback,
    report_cross_validation_scores,
)

@thomasATbayer
thomasATbayer requested a review from SommerKai July 23, 2026 15:18
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.

Make Optuna compatible with hold out set validation

2 participants