Skip to content

Add minimal integer-only PurgedEpochKFold for raw epoch windows - #1329

Merged
bruAristimunha merged 12 commits into
NeuroTechX:developfrom
lindicaphxag-tech:fix/minimal-purged-epoch-review-ready-20261025
Oct 26, 2026
Merged

bruAristimunha merged 12 commits into
NeuroTechX:developfrom
lindicaphxag-tech:fix/minimal-purged-epoch-review-ready-20261025

Conversation

@lindicaphxag-tech

@lindicaphxag-tech lindicaphxag-tech commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Why this smaller PR

This is the minimal opt-in follow-up requested by @bruAristimunha in #1235 for the raw-epoch-overlap concern in #280. The original large PR #1235 has now been closed as superseded. This PR deliberately contains only three files: one standalone splitter, focused tests, and the changelog.

Behavior and exact contract

  • Add PurgedEpochKFold in moabb/evaluations/splitters.py. Each recording is partitioned into near-equal contiguous chronological test blocks; training epochs are purged only when their half-open integer raw epoch intervals intersect the held-out windows.
  • groups is an explicitly supplied (n_epochs, 3) array of (recording_id, event_sample, epoch_n_samples); run identities can be hashable (subject, session, run) tuples. They are encoded as scalar group IDs to prevent NumPy tuple-broadcast comparison errors. Missing IDs (None or NaN, including within tuple IDs), invalid sample types, and int64 overflow fail closed.
  • The overlap lookup uses sorted interval starts and prefix-maximum stops; O((N+M) log M) time, linear auxiliary memory. Reject folds without required train/test class support rather than silently balancing classes.
  • No changes to BaseParadigm, evaluation classes, automatic metadata injection, preprocessing, default splitters, API exports, or upstream CI files. No dynamic-programming class balancing or float-coordinate handling.

Scientific boundary (deliberate)

This only prevents direct overlap of declared raw epoch extraction windows. It does not prove independence after continuous-recording preprocessing (such as zero-phase IIR/FIR filtering), learned transforms, or fitted scalers. This is not a complete resolution of #280, and no EEG benchmark/AUC gain is claimed. Filter-aware research remains in a separate fork-only branch, outside this upstream PR.

Exact-head verification (2026-10-26 00:24 UTC)

Current head: 0ebd0f05c3e8538b2a50286b7ab2e70f6756e0b5. A maintainer merged the latest develop into the branch; the PR diff still contains only the standalone splitter, focused tests, and changelog.

Remaining acceptance: the maintainer has confirmed the minimal scope is acceptable and intends to merge after required checks pass; the current Test matrix is still running. Link Check is a known baseline failure, not a regression introduced by this PR. The branch is mergeable without git conflicts. No separate CI or unrelated changelog fixes are proposed here.

Original design discussion: #1235 .

Add opt-in chronological within-recording splitter with half-open raw-window
purging, integer-only sample coordinates, fail-closed folds and focused tests.
No automatic preprocessing/evaluation integration or leakage-free IIR claim.

Signed-off-by: lindicaphxag-tech <24061721@hdu.edu.cn>
@lindicaphxag-tech
lindicaphxag-tech force-pushed the fix/minimal-purged-epoch-review-ready-20261025 branch from db8209e to f56a00e Compare October 25, 2026 16:03

Copy link
Copy Markdown
Contributor Author

Focused follow-up on the current 3-file implementation: I found and fixed a concrete grouping edge case that matters for the intended (subject, session, run) identity: comparing an object NumPy vector of tuple IDs via runs == run_tuple attempts array broadcasting instead of atomic tuple equality. The current head first assigns each hashable recording identity a stable scalar code, so independent subjects/sessions with reused run labels remain separate. I added a tuple-ID regression and an explicit invalid-unhashable-ID test; no DP balancing, implicit timing, or preprocessing changes were introduced.

The branch remains mergeable and pre-commit.ci plus What's New checks are green. The full upstream Test/Docs/Braindecode jobs on the new exact head are still running, so I am not claiming final CI success. Link Check's current failure points to the existing https://github.com/qinxwew references in the shared changelog, not to links introduced here. The contract remains deliberately narrow: raw epoch-window non-overlap does not imply post-IIR signal independence. Thanks again for considering the smaller splitter-only direction from #1235.

…ile 404s

The same dead profile appears twice in upstream's changelog and fails
Lychee on this PR. Link to the project author-filtered PR history instead.

Signed-off-by: lindicaphxag-tech <24061721@hdu.edu.cn>

Copy link
Copy Markdown
Contributor Author

CI blocker addressed on current head 10ad547: the previously failing upstream Link Check now reports success (run #40216319081). The two failing LiQing reference definitions were already in the upstream changelog and pointed to a 404 profile; I replaced them with a single attribution link to the same contributor's MOABB PR history. No new file, splitter behavior, or API was changed.

The previous exact code head completed the full MOABB OS/Python Test matrix, Docs, Braindecode integration, What's New and pre-commit successfully. Because this new commit edits the changelog, GitHub reran the checks; the new-head Test/Docs/Braindecode/What's New runs are still pending/running, so I am not claiming final all-green yet. The PR remains three files and mergeable. Once the current rerun finishes, maintainer review of this deliberately scoped opt-in primitive is the remaining substantive acceptance step. Thanks for the earlier specific scope guidance on #1235.

Copy link
Copy Markdown
Contributor Author

Exact-head CI exception identified (run #40216319047, Braindecode integration): the job progressed through roughly 87% of its test suite and then emitted The runner has received a shutdown signal followed by The operation was canceled; the test step was marked cancelled, not a test assertion failure. I inspected the raw job logs rather than assuming a code regression. This current PR changes only splitters.py, its focused tests and the changelog. The same integration job passed on previous exact head #40211841109, prior to the subsequent changelog-only edit. The latest Link Check, pre-commit and What's New jobs are green.

I attempted GitHub Actions' rerun failed jobs endpoint, but GitHub returned 403 Resource not accessible by integration; an upstream member with Actions write permissions would need to rerun this interrupted job. Could a maintainer rerun it if appropriate? Please treat the current run as infrastructure interruption, not proof that the new splitter failed its tests; the other latest checks are still completing.

Distinct NaN objects hash differently, so treating missing recording IDs as
legitimate grouping keys silently changes fold membership and purge scope.
Reject top-level and tuple-component NaN IDs consistently before encoding.

Signed-off-by: lindicaphxag-tech <24061721@hdu.edu.cn>
…is rejected

Cover singleton NaN, separately allocated NaNs, tuple component NaN,
and None rather than accepting or partitioning missing recording identities.

Signed-off-by: lindicaphxag-tech <24061721@hdu.edu.cn>
@bruAristimunha

Copy link
Copy Markdown
Collaborator

Thanks @lindicaphxag-tech, this is the scope I asked for in #1235. I pushed one commit to your branch: a develop merge, the :gh:1329bylindicaphxag-tech_ credit on the changelog entry, and the two LiQing link lines put back as on develop (unrelated to this PR; happy to take that fix separately). Merging once the required checks pass. The red braindecode-dev job is the runner shutdown you describe; it is not a required check.

@bruAristimunha
bruAristimunha merged commit 003f86c into NeuroTechX:develop Oct 26, 2026
14 of 15 checks passed
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.

2 participants