Skip to content

Restore acceptance tests on supported Python versions - #1159

Merged
bruAristimunha merged 12 commits into
masterfrom
copilot/return-acceptance-test-across-version
Oct 1, 2026
Merged

bruAristimunha merged 12 commits into
masterfrom
copilot/return-acceptance-test-across-version

Conversation

Copilot AI commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

The acceptance suite was still pinned to Python 3.7, so the main end-to-end tests were skipped on every supported runtime. This updates those tests to run again across supported Python versions without depending on exact floating-point histories.

  • Remove obsolete version gating

    • Drop the sys.version_info == (3, 7) skips from the acceptance tests.
    • Keep the existing network marker behavior intact for PhysioNet-backed tests.
  • Align tests with the current model API

    • Update acceptance tests to construct ShallowFBCSPNet with the current argument names:
      • n_chans
      • n_outputs
      • n_times
  • Make acceptance assertions version-stable

    • Replace exact epoch-by-epoch metric snapshots with shared assertions over training history:
      • expected epoch count is recorded
      • losses remain finite and improve over training
      • accuracies remain finite and within [0, 1]
      • key accuracy signals improve where the test expects learning
    • Preserve stronger structure checks in the EEG classifier acceptance test for per-epoch batch accounting.
  • Shared helper for acceptance history checks

    • Add a small helper to centralize these invariants and keep the acceptance tests consistent.

Example of the assertion style change:

assert_learning_history(
    clf.history,
    n_epochs=4,
    loss_keys=("train_loss", "valid_loss"),
    accuracy_keys=("train_accuracy", "valid_accuracy"),
    improving_accuracy_keys=("train_accuracy", "valid_accuracy"),
)

Copilot AI changed the title [WIP] Investigate and propose improvements for acceptance tests Restore acceptance tests on supported Python versions Sep 8, 2026
Copilot AI requested a review from bruAristimunha September 8, 2026 10:22
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.78%. Comparing base (0af5317) to head (543df40).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1159      +/-   ##
==========================================
+ Coverage   87.73%   87.78%   +0.05%     
==========================================
  Files         149      149              
  Lines       17452    17461       +9     
==========================================
+ Hits        15312    15329      +17     
+ Misses       2140     2132       -8     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bruAristimunha and others added 7 commits September 30, 2026 01:15
Resolutions:
- docs/whats_new.rst: keep master's 1.8.1 section (DIVER1, ZUNA, MSCFormer,
  EEGMiner HPU entries) and re-add the :gh:`1159` acceptance-tests entry at
  the top of Enhancements; a previous attempt had corrupted the RST title
  underlines and was discarded.
- test/acceptance_tests/*: no conflict (master did not touch them).
On Windows, _parse_description_from_file_path normalises the mocked
POSIX file paths with os.path.normpath, so _get_header received a
backslash path and raised KeyError in
test_variable_length_trials_cropped_decoding (Windows 3.12/3.13 CI).
Normalise the lookup key to forward slashes inside the mock and add a
focused unit test that exercises both separators on every host.
test_trialwise_decoding is deterministic for a fixed seed, but it asserted
valid_accuracy[0] < valid_accuracy[-1] and valid_loss[0] > valid_loss[-1]
on a 30-trial validation split after 6 epochs. Those comparisons move by
one trial and failed for 40/160 seeds (25%), so a torch/BLAS/platform change
flips them; locally the fixed seed now gives 0.600 -> 0.567.

Keep the finite/range checks on all metrics and the improving train
loss/accuracy, and add:
- best train loss < 0.8 * first epoch (worst of 160 seeds: 0.651)
- final train_accuracy >= 0.65, i.e. 39/60 (worst of 160 seeds: 43/60)
- predict() accuracy on the train and valid splits equals the final
  history scores

The test now guards the training pipeline (labels, loss, optimizer,
predict and scoring), not generalization. It remains a network test and
is skipped in CI without --run-network.
The acceptance tests asserted first-vs-last history improvements that failed
for 23-32 of 50 seeds, and the cropped tests used nll_loss on logits (the
models no longer end in log_softmax), so their losses went negative and
their held-out accuracy stayed at chance.

- Use BNCI2014_001 subject 1 (left vs right hand), which the ubuntu 3.12
  CI job already downloads, instead of PhysioNet EEGBCI.
- Trialwise: 4-fold CV predicts all 288 trials once while held out; assert
  held-out accuracy >= 0.67, a binomial test against chance, and a
  shuffled-label control trained with the same pipeline <= 0.64. A second
  test fits one fold twice under deterministic algorithms and requires
  identical histories and probabilities.
- Cropped: train on session 1, predict the 144 trials of session 2 with
  cross-entropy and a cosine lr schedule; accuracy >= 0.75, control <= 0.66.
- EEG classifier and variable-length tests: cross-entropy, non-negative
  losses, predict/scoring consistency and measured train-loss checks.

Thresholds come from local sweeps that varied the init, batch-order and fold
seeds separately (trialwise real 0.708-0.812 over 231 runs, control
<= 0.604 over 275; cropped real 0.826-0.882 over 151, control <= 0.590
over 101).
The matrix jobs run pytest without --run-network, so the real-data
acceptance tests were always skipped. Add one ubuntu 3.12 job with
OMP_NUM_THREADS=1 that restores (without saving) the ubuntu 3.12 MNE data
cache and runs test/acceptance_tests with --run-network.
…rials

np.array() on ragged per-trial predictions raises ValueError on NumPy >= 1.24. Equal-length trials still return the same 3-D array.
@bruAristimunha
bruAristimunha marked this pull request as ready for review October 1, 2026 18:52
Copilot AI balanced review requested due to automatic review settings October 1, 2026 18:52
@bruAristimunha
bruAristimunha merged commit 55afc12 into master Oct 1, 2026
10 of 12 checks passed

@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: 543df406f0

ℹ️ 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".

i_stop_in_trials=torch.cat(all_inds[2::3]),
)
ys_per_trial = np.array(ys_per_trial)
ys_per_trial = _stack_trials(ys_per_trial)

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 Document list returns for variable-length targets

When return_targets=True and windows carry sequence targets, unequal trial lengths make this new call return a Python list, but the predict_trials return documentation in this module, classifier.py, and regressor.py still promises that trial_targets is always an np.ndarray. Callers relying on ndarray operations such as .shape will therefore fail for the newly supported variable-length case; update the target return type and description alongside the prediction documentation.

Useful? React with 👍 / 👎.

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.

Copilot review overview

🟡 Changes recommended

The acceptance job omits Python 3.13, and several documented contracts or stated implementation details do not match the changes.

Review effort: Balanced
Findings: 1 Medium severity · 4 Low severity

Open (5)
What changed in this PR

Restores acceptance testing on current runtimes, modernizes decoding tests, and supports variable-length trial predictions.

Changes:

  • Reworks acceptance tests around BNCI data and robust learning assertions.
  • Supports variable-length predict_trials results.
  • Adds deterministic testing, CI coverage, and Windows mock-path handling.
File Description
.github/​workflows/​tests.yml Adds acceptance CI job.
braindecode/​classifier.py Updates trial prediction documentation.
braindecode/​datasets/​tuh.py Normalizes mock Windows paths.
braindecode/​regressor.py Updates trial prediction documentation.
braindecode/​training/​scoring.py Handles variable-length trial arrays.
docs/​whats_new.rst Documents acceptance and prediction changes.
test/​acceptance_tests/​_bnci.py Adds shared BNCI data utilities.
test/​acceptance_tests/​conftest.py Adds deterministic-algorithm fixture.
test/​acceptance_tests/​test_cropped_decoding.py Adds robust cropped-decoding checks.
test/​acceptance_tests/​test_eeg_classifier.py Modernizes classifier acceptance checks.
test/​acceptance_tests/​test_trialwise_decoding.py Adds cross-validation and controls.
test/​acceptance_tests/​test_variable_length_trials_decoding.py Tests variable-length predictions.
test/​unit_tests/​datasets/​test_tuh.py Tests cross-platform mock paths.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Real-data acceptance tests (marked network, so skipped by the jobs above).
# They train on BNCI2014_001 subject 1, which the ubuntu 3.12 job downloads too.
acceptance:
name: acceptance (ubuntu-latest, 3.12)
Comment thread braindecode/classifier.py
Comment on lines +213 to +214
receptive field of the network. If trials have different lengths,
a list with one (n_classes x n_predictions) array per trial.
Comment thread braindecode/regressor.py
Comment on lines +158 to +159
receptive field of the network. If trials have different lengths,
a list with one (n_classes x n_predictions) array per trial.
Comment on lines +425 to +426
receptive field of the network. If trials have different lengths,
a list with one (n_classes x n_predictions) array per trial.
Comment on lines +93 to +97
for key in ("train_loss", "valid_loss"):
# Cross-entropy is non-negative; a negative loss means log-probabilities
# and logits got mixed up.
values = np.asarray(history[:, key])
assert np.all(np.isfinite(values)) and np.all(values >= 0)
bruAristimunha added a commit to mahirjain01/braindecode that referenced this pull request Oct 1, 2026
Resolves docs/whats_new.rst: keeps master's braindecode#1159 and braindecode#1155 entries and adds the AXON (braindecode#1182) entry above them; author link kept. test_foundation_models.py auto-merged.
bruAristimunha added a commit to julien-gadonneix/braindecode that referenced this pull request Oct 1, 2026
Resolutions:
- docs/whats_new.rst: master's file kept as is; the MAPA entry (:gh:`1178`)
  inserted first under Enhancements, above master's braindecode#1159 and braindecode#1155 entries.
bruAristimunha added a commit to Fashad-Ahmed/braindecode that referenced this pull request Oct 1, 2026
Resolve docs/whats_new.rst: keep master's braindecode#1159/braindecode#1155 entries and the SleepFM (braindecode#1106) entry. test_foundation_models.py auto-merged (master's LaBraM tests plus the SleepFM tests); _DIRECT_TORCHSCRIPT_MODELS stays 32 on base, PR and master.
bruAristimunha added a commit to bruAristimunha/braindecode that referenced this pull request Oct 5, 2026
Resolves the conflict from master's braindecode#1155 (LaBraM time embedding +
mean-pooling) landing after this PR's branch point.

- docs/whats_new.rst: kept both bug-fix entries (braindecode#1194 qkv routing,
  braindecode#1159 predict_trials variable-length fix), concatenated.
- braindecode/models/labram.py: auto-merged cleanly by git; verified
  both master's time_embed/use_mean_pooling additions and the PR's
  self.qkv(x) module-call routing (replacing F.linear on qkv.weight)
  are present together.
- test/unit_tests/models/test_foundation_models.py: auto-merged
  cleanly, both sides' additions kept.
- All other changed files (.github/workflows/tests.yml,
  braindecode/classifier.py, braindecode/datasets/tuh.py,
  braindecode/regressor.py, braindecode/training/losses.py,
  braindecode/training/scoring.py, test/acceptance_tests/*,
  test/unit_tests/datasets/test_tuh.py,
  test/unit_tests/test_eegneuralnet.py,
  test/unit_tests/training/test_losses.py) are master-side changes
  since the PR's branch point, auto-merged without conflict.
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.

Return the acceptance test across version

3 participants