Repository navigation
Restore acceptance tests on supported Python versions - #1159
Conversation
Co-authored-by: bruAristimunha <[email protected]>
Co-authored-by: bruAristimunha <[email protected]>
Codecov Report❌ Patch coverage is 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:
|
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.
There was a problem hiding this comment.
💡 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) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
Open (5)
Matrix network acceptance tests across all supported Python versions · New Update target return documentation for variable-length sequences · New Update target return documentation for variable-length sequences · New Document list return type for variable-length targets · New Centralize repeated learning history assertions or update description · New
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_trialsresults. - 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) |
| receptive field of the network. If trials have different lengths, | ||
| a list with one (n_classes x n_predictions) array per trial. |
| receptive field of the network. If trials have different lengths, | ||
| a list with one (n_classes x n_predictions) array per trial. |
| receptive field of the network. If trials have different lengths, | ||
| a list with one (n_classes x n_predictions) array per trial. |
| 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) |
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.
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.
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.
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.


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
sys.version_info == (3, 7)skips from the acceptance tests.networkmarker behavior intact for PhysioNet-backed tests.Align tests with the current model API
ShallowFBCSPNetwith the current argument names:n_chansn_outputsn_timesMake acceptance assertions version-stable
[0, 1]Shared helper for acceptance history checks
Example of the assertion style change: