Skip to content

Prevent overlapping ERP epoch windows in chronological within-session CV - #1235

Closed
lindicaphxag-tech wants to merge 56 commits into
NeuroTechX:developfrom
lindicaphxag-tech:fix/erp-purged-within-session-280
Closed

lindicaphxag-tech wants to merge 56 commits into
NeuroTechX:developfrom
lindicaphxag-tech:fix/erp-purged-within-session-280

Conversation

@lindicaphxag-tech

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

Copy link
Copy Markdown
Contributor

Maintainer review (2026-10-24): this 13-file implementation is not the requested merge shape. The maintainer asks for a smaller standalone splitter, with chronological within-run test blocks and exact half-open raw-epoch interval purging only; drop the DP class-balancing search, float16/float32 special handling, extra CI workflow, and paradigm/evaluation plumbing. They offered a prompt review of a new, genuinely narrowed PR and are leaving this discussion open. The existing fork-only Draft #6 is not yet the requested final shape (it still contains DP), and is not a second upstream PR. Do not treat prior green tests as maintainer acceptance. The current branch also conflicts with develop. Current link-check failure references pre-existing https://github.com/qinxwew in upstream docs/source/whats_new.rst, not a link introduced by this PR. No duplicate upstream PR will be opened without an independently validated, clearly distinct narrow diff.

Summary

Addresses the direct shared-raw-sample failure described in #280: an opt-in chronological within-session CV splitter that removes overlapping epoch extraction intervals from train/test folds. It does not guarantee post-filter signal independence.

Problem

Random trial-wise CV can put epochs that share samples from the same continuous recording into train and test. With a 500-sample epoch and 100-sample SOA, trial-disjoint folds are not signal-disjoint.

Approach

  • add PurgedEpochKFold, a sklearn-compatible group-aware splitter;
  • sort epochs by true processed event sample within each run;
  • form contiguous test blocks per run, choosing near-equal boundaries to improve class balance without shuffling epochs;
  • purge candidate training epochs whose half-open sample interval overlaps any test interval from the same run;
  • never purge across recording boundaries, using the available (subject, session, run) identity so reused run names cannot cause false cross-session/subject purges;
  • fail closed if true event timing is absent or a fold lacks class support;
  • expose event_sample and epoch_n_samples only on demand, preserving the default paradigm metadata schema;
  • auto-route timing metadata when WithinSessionEvaluation(..., cv_class=PurgedEpochKFold) is selected;
  • report n_purged_overlap and purge_fraction as per-fold protocol diagnostics.

Exact large-integer interval correctness (current head)

The original sample parser passed integer event/sample-window coordinates through float64; above 2**53 that can collapse distinct integer sample indices and invalidate exact half-open overlap decisions. fb067c4 preserves true int64 sample identity, refuses ambiguous large floating inputs, and prevents overflow when constructing interval stops. d0100fc adds exact translation-invariance, boundary and fail-closed regressions; pre-commit formatting 45a35dc was an earlier review head. The latest source has additional floating-dtype provenance validation.

Independent exact-source native CI: Python 3.11/3.13 focused PurgedEpochKFold tests both SUCCESS. That workflow checks out the immutable 45a35dc SHA and runs the actual moabb/tests/test_splits.py -k purged_epoch tests, rather than duplicating the parser in a toy implementation. The official full test matrix on the current head runs separately; focused green tests do not replace full upstream acceptance.

Floating-point precision provenance (latest bot-formatted Head fc75f39)

Even valid finite integer-looking timing values can already have lost one sample of precision in their original input type. float32(2**24+1) equals float32(2**24); float16(2**11+1) similarly aliases float16(2**11). A further trap: pandas.DataFrame.to_numpy(dtype=object) can widen NumPy and nullable Float32 columns into Python floats, erasing their ORIGINAL precision and allowing a too-late float64-only check to miss the collision.

The new source changes 1940231 and nullable-float extension c0d742e fail closed before dtype widening and also guard direct object-array NumPy float scalar inputs. Five original tests and two nullable-pandas tests cover lost low-precision integer identity, missing samples, safe small floats, direct bypass attempts and interval widths. Bot commit fc75f39 only reformatted two test statements, with no behavior change.

First exact-source run 27dd803 passed BOTH Python 3.11 and 3.13. Independent validation of latest bot-formatted fc75f39 is running separately from the latest full MOABB upstream matrix. All regression evidence is on actual splitter source, not a fabricated reproduction. The existing preprocessing/IIR scope caveat remains unchanged.

Reused run-label correctness

Run labels like "0" are routinely reused across subjects or sessions. The original splitter grouped solely by run, which could place independent recordings on the same sample axis and incorrectly remove training epochs that merely have overlapping numeric sample indices from different raw files. Source fix dfa052a encodes the available (subject, session, run) tuple without changing the three-column groups shape or default metadata. Regression cfca859 verifies independent sessions, subjects and refusal of missing recording identity. The cross-recording fix first reached bot-formatted Head 3c76a2c. Independent immutable-SHA validation passed both Python 3.11 and 3.13 on the real moabb/tests/test_splits.py -k purged_epoch regression subset. The upstream full cross-platform matrix and maintainer Review are separate gates.

Minimal usage

from moabb.evaluations import PurgedEpochKFold, WithinSessionEvaluation

evaluation = WithinSessionEvaluation(
    paradigm=paradigm,
    datasets=[dataset],
    cv_class=PurgedEpochKFold,
    n_splits=5,
)

No manual chronology labels are required; the evaluation requests event timing from the processed epoch stream.

Regression design

The synthetic witness uses 100-sample SOA and 500-sample epochs:

  • shuffled StratifiedKFold has train/test interval overlap;
  • PurgedEpochKFold has zero same-run overlap;
  • every epoch appears in a test fold exactly once;
  • test folds remain contiguous in event time;
  • when feasible, boundary selection improves class balance while preserving class support;
  • independent runs never purge one another;
  • missing/invalid timing fails explicitly;
  • BaseParadigm.get_data() keeps its existing subject/session/run metadata by default and only adds timing when requested.

Scope / limitation

Important preprocessing caveat discovered in review: BaseParadigm.make_process_pipelines() applies the standard continuous-Raw IIR band-pass before cutting epochs. A zero-phase IIR filter has nonlocal temporal response, so disjoint extraction intervals do not prove that the resulting filtered signals contain no cross-boundary contribution. This PR prevents direct reuse of overlapping raw sample-index windows; it does not claim strict signal independence or eliminate all possible temporal leakage. A stricter protocol requires fold-aware preprocessing or independently justified transformation-specific support. A new synthetic regression (moabb/tests/test_purged_preprocessing_boundary.py) demonstrates this counterexample explicitly and preserves the narrower contract.

The v1 contract assumes a fixed event-locked epoch window within an evaluated partition. The interval is represented on the processed epoch sample grid. It intentionally does not infer chronology from DataFrame row order and does not claim to model arbitrary time-warping post-processing. If a chronological partition cannot provide class support in every fold, the splitter fails closed rather than interleaving time blocks.

The review suggestion to use time-split CV with stratification is addressed by selecting near-equal contiguous boundaries with dynamic programming; temporal order remains intact and overlap is purged exactly. Earlier heads completed the full upstream matrix. The exact fc75f39 precision-provenance head passed the official full test matrix on Ubuntu Python 3.11–3.14, macOS Python 3.12, and Windows Python 3.12, plus Docs, link/news checks and Braindecode integration. Maintainer approval of the API/scope is still required. The four runtime files carry timing metadata only for this opt-in evaluation path; default metadata remains unchanged. This PR is ready for review.

Exact-head validation update (2026-10-13): The current head fc75f39b498b44db5b8663ddaa05d30cb0a51a32 has completed the official Test matrix successfully on Ubuntu Python 3.11/3.12/3.13/3.14, Windows Python 3.12, and macOS Python 3.12. Docs, news check, link check, and braindecode-dev integration are also successful. No fresh CI/merge-conflict blocker is known; the outstanding gate is maintainer approval of the API/scope. The guarantee is exclusively direct raw-epoch interval nonoverlap, not post-filtered signal independence (whole-recording IIR preprocessing remains an explicit limitation). The narrow alternative exists only as a fork Draft, not as a competing upstream PR.

@byneuro

byneuro commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator

I am not super supportive of this design, like, I feel that we can have the same thing just using the native split or implement some time split CV with stratification. The PR is also touch many extra sensitive areas of moabb.

@lindicaphxag-tech

Copy link
Copy Markdown
Contributor Author

Thanks for the design feedback. I checked the native alternatives against issue #280: GroupKFold by run prevents mixing separate recordings, but it cannot split the single-run case in the issue; ordinary StratifiedKFold shuffles epochs and reintroduces sample overlap. This draft instead makes contiguous time blocks within each run, purges every half-open interval overlap, and fails closed when a fold lacks class support. The timing columns are opt-in, so the default paradigm metadata remains unchanged; the evaluation path is activated only for this splitter.

I agree the cross-cutting integration needs a clear scope boundary. Would you prefer this issue to cover only run-held-out evaluation (which leaves single-run recordings unresolved), or is the opt-in single-run time/purge protocol in scope? I am keeping the PR as draft until that design question is resolved.

I also fixed the two failures from the first full CI run: MNE offsets event samples when concatenating runs, so the regression now compares timing within each run; and the cross-run purge test now uses valid non-overlapping epochs with an explicit cross-run overlap candidate. I moved metadata assertions back into their original test. On the new head 61fe1c453262782da079880d9e0b26049f5c4450, all three affected test modules passed locally under Python 3.11, and pre-commit passed for the changed files.

@lindicaphxag-tech

Copy link
Copy Markdown
Contributor Author

Update on the new head: I merged current develop and resolved the changelog conflict while preserving both entries. GitHub now reports the branch mergeable; pre-commit, link-check, and changelog checks pass. The full cross-platform test matrix has started and is pending. Focused full-file validation remains green after the base sync: test_paradigms.py, test_splits.py, and test_evaluations.py all passed under Python 3.11.

@lindicaphxag-tech

Copy link
Copy Markdown
Contributor Author

CI update for head 4c5b03592f16a210d25218714887e2ce6d3ae9ae: the main test matrix has now passed on Ubuntu Python 3.11, 3.12, 3.13, and 3.14, plus macOS and Windows on Python 3.12; pre-commit, link-check, changelog, and Codecov are also green. The docs build and braindecode-dev job are still running. I am leaving the PR in draft until the design scope in my earlier reply is resolved.

@lindicaphxag-tech

Copy link
Copy Markdown
Contributor Author

I followed up on the stratified-time-split suggestion in commit 0a38c741c6d3151a0cff88dfc530cc1e2fb6f033. The splitter now uses dynamic programming to shift near-equal fold boundaries for better class balance while keeping each test fold contiguous in event time; it still purges exact half-open sample overlap and fails closed when class support cannot be maintained. A regression witness compares the class-proportion error against equal-size temporal chunks and checks contiguity, full test coverage, class support, and zero overlap. Local validation on this head: all of test_splits.py and test_evaluations.py passed; Ruff check/format passed. New upstream CI is running. Does this interpretation of stratified time CV address the design concern? I am keeping the PR draft until we get scope guidance.

@lindicaphxag-tech

Copy link
Copy Markdown
Contributor Author

CI has advanced on current head 0a38c741c6d3151a0cff88dfc530cc1e2fb6f033: Ubuntu Python 3.11 and 3.14 are green, as are Codecov, pre-commit, link-check, and changelog validation. Ubuntu 3.12/3.13, macOS, Windows, docs, and braindecode-dev remain in progress. The PR remains draft pending the design-scope response requested above.

@lindicaphxag-tech
lindicaphxag-tech marked this pull request as ready for review October 7, 2026 22:38

Copy link
Copy Markdown
Contributor Author

I’ve made the draft ready for review. The stratified-time suggestion is implemented with dynamic-programming boundary selection while keeping each test fold contiguous and purging exact sample overlap. The complete upstream matrix is green now: Ubuntu Python 3.11–3.14, macOS/Windows Python 3.12, docs, and the braindecode-dev job; pre-commit, link, changelog, and Codecov checks also pass. Event timing is opt-in, so the default paradigm metadata schema is unchanged. Please review the updated scope and let me know if you want a narrower integration.

@byneuro

byneuro commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Can you do some visual, ala scikit learn, to represent exactly what this split is? I have the feeling that we are making a mistake here, but I may be wrong.

Copy link
Copy Markdown
Contributor Author

A concrete narrower implementation is available for your scope review: fork-only Draft PR #6 (not a competing upstream submission). Since the earlier version, I corrected a scientifically meaningful recording-identity issue: identical run labels from distinct subjects/sessions no longer share a sample coordinate system or accidentally purge each other's epochs. I also added an independent 30k-interval sweep-line oracle and dedicated Python 3.11/3.13 targeted CI; both focused jobs pass, with 13/14 current full-fork checks successful and docs still running. This alternative changes only splitter core, tests, changelog, and a focused workflow; no paradigm/evaluator/preprocessing/default behavior. The only guarantee is direct raw epoch extraction-window non-overlap, not post-IIR signal independence. Would you prefer this opt-in isolated primitive as a smaller first upstream change, or an end-to-end fold-aware preprocessing design for #280? I will avoid opening a duplicate upstream PR until you advise.

lindicaphxag-tech added a commit to lindicaphxag-tech/moabb that referenced this pull request Oct 12, 2026
lindicaphxag-tech added a commit to lindicaphxag-tech/moabb that referenced this pull request Oct 12, 2026
lindicaphxag-tech added a commit to lindicaphxag-tech/moabb that referenced this pull request Oct 12, 2026
lindicaphxag-tech added a commit to lindicaphxag-tech/moabb that referenced this pull request Oct 12, 2026

@bruAristimunha bruAristimunha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @lindicaphxag-tech for the work, the figure and the honest scope note on filtering. One review round on the current head (the branch also conflicts with develop now):

The problem in #280 is real, but this version is too large for what it guarantees: about 350 lines in splitters.py (a dynamic-programming boundary search and extensive float16/float32 coordinate handling), a new include_epoch_timing option in BaseParadigm.get_data, and evaluation changes to inject the groups. And, as the docstring says, it still does not isolate train and test after the continuous-recording filter, which is the main leakage path for overlapping ERP epochs in MOABB.

The splitter-only shape of your fork draft (lindicaphxag-tech#6, no change to BaseParadigm.get_data or the evaluations) is the right direction: a standalone splitter that takes the event timing as groups, used through cv_class / cv_kwargs. Trimmed further:

  • keep only the contiguous within-run blocks and the half-open overlap purge;
  • drop the DP class balancing and the extra CI workflow (fail with a clear message when a fold lacks a class, as you already do) and the float16/float32 special cases (accept integer sample columns, reject the rest);
  • document the filtering limitation in the class docstring, as now.

If you open that as a new PR, I'll review it quickly. I'll leave this one open until then so the discussion stays linked.

# Conflicts:
#	docs/source/whats_new.rst
#	moabb/tests/test_evaluations.py
#	moabb/tests/test_splits.py

Copy link
Copy Markdown
Contributor Author

Following the requested scope reduction, I opened #1329 as a new standalone upstream PR. It is built from current develop and changes only (1) an integer-only PurgedEpochKFold in splitters.py, (2) focused regressions, and (3) one changelog entry. It uses near-equal contiguous within-recording blocks, exact half-open raw-window overlap purging, and fails closed on absent classes; no DP stratification, float16/float32 special cases, automatic paradigm/evaluator changes, or extra CI workflow. The pre-commit gate and What's New check passed; the full OS/Python matrix is still executing. We explicitly retain the continuous-IIR filtering leakage limitation. GitHub now reports no merge conflict on #1329.

The independent Link Check currently fails only on two existing github.com/qinxwew 404 citations in the shared changelog, not on newly added URLs; I have left unrelated attribution untouched to keep this correction narrowly reviewable. Thanks for the concrete guidance — the new #1329 is the requested review target; this earlier #1235 remains the design-history reference.

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.

3 participants