Remove redundant learning curve implementation paths#962
Merged
bruAristimunha merged 7 commits intounify-split-logicfrom Feb 3, 2026
Merged
Remove redundant learning curve implementation paths#962bruAristimunha merged 7 commits intounify-split-logicfrom
bruAristimunha merged 7 commits intounify-split-logicfrom
Conversation
Collaborator
|
Please address this comments too: #961 (comment) |
Copilot
AI
changed the title
[WIP] Unify split logic and add LearningCurveSplitter
Remove redundant learning curve implementation paths
Feb 3, 2026
Collaborator
|
Please remove the redundancy |
Collaborator
|
@copilot Please remove the redundancy |
- Remove data_size and n_perms parameters from WithinSessionEvaluation - Remove get_data_size_subsets(), score_explicit(), and _evaluate_learning_curve() methods - Simplify evaluate() to always use _evaluate() path - Update docstring to recommend cv_class=LearningCurveSplitter - Update all examples to use new LearningCurveSplitter API - Update tests to use new API Co-authored-by: bruAristimunha <[email protected]>
Contributor
Author
Removed in commit c4ec348. Eliminated ~200 lines of duplicate code by removing the legacy
Done in c4ec348. All redundant methods removed:
All examples and tests updated to use the new API.
Addressed in c4ec348 - removed all redundant learning curve code as discussed. The legacy |
Collaborator
|
Fix the pre-commit and any test issue @copilot |
- Apply black formatting (line length 90) - Fix import sorting with isort - Remove unused imports (Optional, StratifiedShuffleSplit) - Apply ruff fixes Co-authored-by: bruAristimunha <[email protected]>
- Remove extra blank lines per black style guide Co-authored-by: bruAristimunha <[email protected]>
- Apply isort to properly order imports in all changed files Co-authored-by: bruAristimunha <[email protected]>
- Apply black formatting after isort - Remove extra blank line after imports
Contributor
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29c4a7a8c3
ℹ️ 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".
- Skip splits where training set collapses to single class - Log warning when splits are skipped due to single-class training sets - Fix ArrowStringArray shuffle warnings by converting to numpy arrays - Update tests to call process() since validation happens at evaluation time - Fix isort import ordering in learning curve examples
3 tasks
bruAristimunha
added a commit
that referenced
this pull request
Feb 4, 2026
* Remove redundant learning curve implementation paths (#962) * Initial plan * Remove redundant learning curve implementation - Remove data_size and n_perms parameters from WithinSessionEvaluation - Remove get_data_size_subsets(), score_explicit(), and _evaluate_learning_curve() methods - Simplify evaluate() to always use _evaluate() path - Update docstring to recommend cv_class=LearningCurveSplitter - Update all examples to use new LearningCurveSplitter API - Update tests to use new API Co-authored-by: bruAristimunha <[email protected]> * Fix pre-commit linting issues - Apply black formatting (line length 90) - Fix import sorting with isort - Remove unused imports (Optional, StratifiedShuffleSplit) - Apply ruff fixes Co-authored-by: bruAristimunha <[email protected]> * Apply final black formatting fixes - Remove extra blank lines per black style guide Co-authored-by: bruAristimunha <[email protected]> * Fix isort import ordering - Apply isort to properly order imports in all changed files Co-authored-by: bruAristimunha <[email protected]> * Resolve black/isort formatting conflict - Apply black formatting after isort - Remove extra blank line after imports * Add single-class safeguard for LearningCurveSplitter - Skip splits where training set collapses to single class - Log warning when splits are skipped due to single-class training sets - Fix ArrowStringArray shuffle warnings by converting to numpy arrays - Update tests to call process() since validation happens at evaluation time - Fix isort import ordering in learning curve examples --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: bruAristimunha <[email protected]> Co-authored-by: Bru <[email protected]> * Add whats_new entries for learning curve unification Document the learning curve and splitter improvements: - cv_class and cv_kwargs parameters for all evaluation classes - LearningCurveSplitter for sklearn-compatible learning curves - Removal of data_size and n_perms from WithinSessionEvaluation - Automatic metadata columns for learning curve results - Centralized CV resolution with _resolve_cv() method - Removal of redundant learning curve methods * Add parametrized test for LearningCurveSplitter as cv_class Test that LearningCurveSplitter can be used as cv_class parameter for all main splitters: WithinSessionSplitter, WithinSubjectSplitter, CrossSessionSplitter, and CrossSubjectSplitter. * updating the python files * solving problem with new pandas * updating the splits to make sure about the logic * iteration * Simplify the logic * solving the group problem * iteration 2 * simplify and simplify * Update whats_new.rst with _load_data, _get_nchan, and splitter hoisting Document the extraction of _load_data() and _get_nchan() helpers into BaseEvaluation, the move of _pipeline_requires_epochs() to utils.py, and the WithinSessionSplitter creation hoisted outside the session loop. --------- Co-authored-by: Copilot <[email protected]>
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.
Summary: Redundancy Removal Complete ✅
Pre-commit Formatting Status
Final linter results:
Note: In the pre-commit pipeline, isort runs first, then black applies final formatting. This is the standard practice and the current state matches what pre-commit will produce.
Changes Summary
Code changes:
cv_class=LearningCurveSplitterLinting fixes (6 commits):
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.