Skip to content

Apply cross-validation settings consistently - #1130

Draft
stanbot8 wants to merge 1 commit into
NeuroTechX:developfrom
stanbot8:apply-cross-validation-settings
Draft

Apply cross-validation settings consistently#1130
stanbot8 wants to merge 1 commit into
NeuroTechX:developfrom
stanbot8:apply-cross-validation-settings

Conversation

@stanbot8

@stanbot8 stanbot8 commented Aug 2, 2026

Copy link
Copy Markdown

Evaluation splitter construction silently discards valid cross-validation settings.

This change applies caller cross-validation settings during splitter construction.

Closes #1110.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f8b23e3db

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +89 to 91
"shuffle": True,
"random_state": self.random_state,
**cv_kwargs,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clear random_state when disabling shuffle

When a within-session evaluation has random_state set and supplies cv_kwargs={"shuffle": False}, this merge retains the evaluation's non-None random state while overriding only shuffle. WithinSessionSplitter.__init__ then raises ValueError("random_state should be None when shuffle is False"), so the newly supported shuffle override cannot be used with an otherwise valid seeded evaluation. The identical merge in WithinSubjectEvaluation._create_splitter has the same failure; omit the inherited seed when the effective shuffle setting is false.

Useful? React with 👍 / 👎.

Comment thread moabb/evaluations/base.py
Comment on lines +549 to +550
cv_kwargs = {
name: value for name, value in cv_kwargs.items() if name in parameters

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve defaults for CV constructors accepting kwargs

When a custom cross-validator constructor accepts forwarded options through **kwargs rather than naming every parameter, this filter incorrectly treats all defaults as incompatible. For example, a GroupKFold subclass with __init__(self, **kwargs) can accept n_splits, but CrossSubjectEvaluation(n_splits=3, cv_class=Subclass) drops that value here and silently uses the subclass's underlying default fold count instead. Detect a variadic keyword parameter and retain compatible defaults in that case.

Useful? React with 👍 / 👎.

@stanbot8
stanbot8 marked this pull request as draft August 2, 2026 18:48
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.

Inconsistent n_splits / cv_kwargs handling in BaseEvaluation._resolve_cv and Within* evaluations

1 participant