Fix label leakage in BNCI2025_001, the two montages gh-700 never shipped, and the NEMAR prefetch guard - #1161
Conversation
BNCI2025_001: the EEGLAB files carry seven non-EEG channels alongside the montage -- hand kinematics (x, y, vx, vy), the reaching target position (targetPosX, targetPoxY) and a validity flag. `read_raw_eeglab` types anything it cannot recognise as `eeg`, and nothing in the loader corrected that, so `pick_channels_for_modalities` picked all of them and every paradigm fed the target position and hand velocity to the classifier as features. Those channels encode the direction and distance that make up the class labels. Accuracies reported on this dataset will drop; that is the leak being removed. The sibling loader in the same module (`_convert_run_002_2025`) already builds explicit ch_types, so this was an oversight rather than a decision. The declared metadata was inconsistent too: `n_channels=67` against a 60-electrode montage. Cattan2019_PHMD: NeuroTechXgh-700 announced "fixing unit issue and lack of montage" for this dataset, but the commit only scaled the units -- the loader has no `set_montage` call at all. Its channel list also spells the two frontal-central electrodes `Fc5`/`Fc6`, which are the only two of its sixteen names `standard_1020` cannot resolve, so the montage could not have been applied as written. Cattan2019_PHMD interval: each marker begins a one-minute relaxation block, as the dataset's own `block_duration_s=60.0` records, but `interval=[0, 1]` made `SetRawAnnotations` stamp every block as one second long. `interval[0]` is unchanged, so annotation onsets do not move, and `RestingStateToP300Adapter` passes an explicit `tmax`, so its default path is unaffected. NEMAR prefetch: the guard returned as soon as the store held anything, but the deposit's provenance manifest lands inside the store -- so a caller loading one subject at a time prefetched only the first and silently reached the upstream host for every subject after it, even with the provider pinned to "nemar". Each requested subject is now prefetched; `sourcedata_path` trusts files already on disk, so this costs a listing per subject rather than a re-download.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 195eecb9d6
ℹ️ 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".
| return | ||
| for subject in subjects: | ||
| try: | ||
| self.sourcedata_path(subject=subject, verbose=verbose) |
There was a problem hiding this comment.
Preserve offline reads from a populated NEMAR store
When the requested files are already present locally, this unconditional call still runs nemar.download, whose trust_existing=True skips file transfer only after fetching remote metadata. Consequently, get_data() now touches the network on every load and, with the provider pinned to "nemar", cannot use an already-downloaded dataset during a transient outage or offline session. This also breaks the existing test_get_data_reads_the_nemar_store_end_to_end, which populates the store and explicitly forbids network access; the guard needs to check completeness for each requested subject rather than treating every subject as requiring a remote request.
Useful? React with 👍 / 👎.
Rodrigues2017 has the same defect as Cattan2019_PHMD and was named in the same NeuroTechXgh-700 changelog entry: identical 16-electrode setup, the same `Fc5`/`Fc6` spelling that `standard_1020` cannot resolve, and no `set_montage` call anywhere in the loader. Its METADATA additionally declared `standard_1010`, which is not a montage MNE can build at all. Auditing the rest of that NeuroTechXgh-700 list: Huebner2018, BaseCastillos2023 and Ofner2017 do set a montage, so Rodrigues2017 and Cattan2019_PHMD were the two that slipped. Version: this batch is bug fixes only, so the next release is a patch. Sets 1.6.1dev0 and renames the open changelog section accordingly -- the 1.7.0dev0 bump assumed a minor release that this content does not justify.
The first cut of this fix simply dropped the whole-store early return, so every subject went through `sourcedata_path`. That is correct but costly, and my docstring claiming it was free was wrong: `nemar.download` walks index, version, metadata and manifest before it ever consults `trust_existing`, so a call with nothing to do is still four round-trips. It also broke `test_get_data_reads_the_nemar_store_end_to_end`, which forbids network access and populates a store by hand -- encoding the contract that a non-empty store is trusted as-is. `nemar_sourcedata_is_local` settles the question offline instead. With a cached provenance manifest that attributes files to subjects it checks that subject's files specifically, which is what lets a per-subject caller skip only what it really has. When the manifest cannot answer -- absent, unreadable, or predating the subject field -- it falls back to the older whole-store rule, so the hand-populated store in that test is still trusted. The manifest itself is not counted as data, so a store holding only a manifest is not mistaken for a complete one; that distinction is what fixes the original bug. Adds regression coverage for both branches.
Reuse / simplification:
- Extract `_manifest_subject_files(record, subject)`; the offline check had
copied ~15 lines of manifest parsing out of `_sourcedata_files_for_subject`.
It takes the parsed record so each caller keeps its own error policy -- an
earlier cut of this swallowed OSError and silently turned an unreadable
manifest into a whole-tree download.
- Extract `_sourcedata_store_holds_data(target_dir)` and call it from
`nemar_sourcedata_dl` too, which had its own slightly different copy: it
counted a bare sub-directory as data, so the writer could call a store
fetched that the reader would not call local.
- `sourcedata_path` now calls `_nemar_subject_ids` instead of repeating it.
- `_NON_EEG_CHANNELS_001_2025` is a tuple: every value was "misc". The
`if ch in raw.ch_names` filter stays -- `set_channel_types` raises on a name
that is not in `info`.
Efficiency (measured on an 87-subject store):
- `_sourcedata_store_holds_data` walks depth-first with `os.walk` instead of
`rglob("*")` plus a `stat` per entry. `rglob` is breadth-first, so it
enumerated 353 directories before the first file: 2506 us -> 248 us. This
also repairs the call site in `nemar_sourcedata_dl`, which the extraction
had otherwise slowed down.
- `nemar_sourcedata_is_local` takes the resolved store root, hoisted out of
the per-subject loop. Resolving it re-reads MNE's config file four times,
which was 60% of the per-subject cost.
Correctness caught by the suite while doing the above:
- `_nemar_subject_ids` must return the *scalar* unless the deposit files the
subject under a second label. Returning `[subject]` unconditionally changed
what `nemar_sourcedata_dl` receives and broke the per-subject fallback.
Also makes both `set_montage` calls strict: after the FC5/FC6 correction all
16 channels resolve, so `on_missing="ignore"` would only hide the next typo --
which is how this bug survived since NeuroTechXgh-700.
The label-leak fix was incomplete. `read_raw_eeglab` defaults to `eog=()`,
and this file's chanlocs types are exactly the ones MNE fails to recognise --
the premise of the whole fix -- so EOGL1/EOGL2/EOGL3/EOGR1 stayed `eeg` and
paradigms still picked 64 channels, not 60. The declared
`channel_types={"eeg": 60, "eog": 4, "misc": 7}` was therefore an unverifiable
claim. They are now named to `read_raw_eeglab`, which makes it true by
construction, and `targetPosY` is accepted alongside the published
misspelling so a corrected re-release cannot silently re-open the leak.
`set_channel_types` also warned about the deliberate V -> NA unit change once
per subject; silenced as `lenaig2026` already does.
The offline NEMAR check assumed more than the manifest guarantees. It now
coerces `target_dir` (both natural sources return `str`), ignores non-dict
entries, and treats a drifted schema as "cannot answer" rather than letting an
AttributeError escape `get_data()`. File presence requires an actual file
inside the store: a directory no longer passes, and an absolute manifest name
can no longer make `/` discard the store root.
The two new tests resolved the store through MNE's config without isolation,
which wrote `MNE_DATA` into the developer's real `~/.mne/mne-python.json`.
They now take `_isolated_mne_config`, like the five neighbouring tests.
Also corrects the changelog, which overstated this fix. Every deposit today
has a manifest predating the `subject` field, and those are fetched as a whole
tree -- so the old whole-store rule was already right for them and is kept.
The per-subject check matters once manifests record subjects, and today for a
store left partial by an interrupted fetch.
…setup Rodrigues2017, Cattan2019_PHMD and Cattan2019_VR are the same g.tec recording setup, and all three spelled the two frontal-central electrodes Fc5/Fc6 -- the only two names standard_1020 cannot resolve. Fixing the first two alone left MOABB shipping two spellings of one electrode: a paradigm asking for `channels=["FC5", "FC6", "Cz"]` would select on Rodrigues2017 and Cattan2019_PHMD and trip the `len(picks) == len(self.channels)` assert on Cattan2019_VR, and a benchmark spanning them would lose both electrodes to the channel intersection. That divergence is worse than the original consistent defect. The montage is now set once on the shared Info rather than on each Raw. The `if not ds.code == "Cattan2019-VR"` branch exists for the VR block/repetition splitting, but as a side effect its Raws -- built in a loop further down -- were the only ones in the module that never got channel positions. Setting it on the Info both branches share removes that asymmetry instead of adding a second special case. Also corrects five montage declarations MNE cannot build: "10-10" (x3) and "standard_1010" (x2). `make_standard_montage` rejects both, so anyone taking the metadata at face value got a ValueError.
4aa0404 to
275eba1
Compare
AcquisitionMetadata stored `n_channels` next to `channel_types`, which
describes the same fact. The two could disagree, and for 34 of the 147
catalogued datasets they did.
The cause was not carelessness: the field was being used with two different
meanings. Dreyer2023A stored 27, counting only its EEG electrodes, while its
own channel_types ({eeg: 27, emg: 2, eog: 3}) and its own 32-entry sensors
list both describe 32 channels. 48 datasets declare more than one channel
type, so the two readings diverge for a third of the catalogue.
Rather than add a validator for a field that should not exist, the field is
gone and `n_channels` is a property over `channel_types`. It now consistently
means every recorded channel -- the reading that `sensors` and
`test_n_channels_matches_raw_data` (which counts every non-stim channel)
already assumed. The 34 disagreements are not fixed so much as made
impossible, and `validate_metadata_against_dataset` needs no new assertion.
218 `n_channels=` arguments come out across 93 files, together with the
enrichment branch in metadata/__init__.py whose only job was to backfill the
value. One test asserted the old EEG-only reading for Dreyer2023A and now
asserts the total.
Net -265 lines. Split out of NeuroTechXgh-1161 so the mechanical change is reviewable
on its own; closes NeuroTechXgh-1163.
`moabb.analysis.plotting` applies a seaborn theme to the global rcParams at import time, and `moabb/analysis/__init__.py` imported it eagerly. So merely importing a dataset changed font.family, axes.grid and axes.spines.* for every figure the caller drew afterwards -- values matplotlib reads at axes creation, so a caller cannot undo them after the fact. Tracing it with an import hook rather than by inspection was what found the path, which is indirect: moabb/datasets/bids_interface.py imports moabb.analysis.results for get_digest, which runs moabb/analysis/__init__.py, which imported plotting. - moabb/analysis/__init__.py: the module alias moves into `analyze()`, its only user, and the three re-exported plotting functions are served by `__getattr__` with a matching `__dir__`. - moabb/datasets/utils.py: matplotlib and the plotting helpers are imported inside the two functions that use them. Deliberately NOT done: a PEP 562 `__getattr__` for `benchmark` on the top-level package. I tried it, and it is a trap -- `moabb/benchmark.py` is a module and `benchmark` is a function inside it, so the eager `from .benchmark import benchmark` is what shadows the submodule. Under lazy loading, anything that imports the submodule rebinds `moabb.benchmark` to the module and callers get "TypeError: 'module' object is not callable"; it broke six tests in test_benchmark.py. It is also unnecessary: cutting `analysis/__init__.py` breaks that chain too, since it ran through `moabb.analysis`. Also corrects the download job's path: it ran `pytest ... download.py` from moabb/tests, a file that does not exist, so pytest exited 4 and the job has failed every month since at least March while no @pytest.mark.download test ran anywhere in CI. The corrected path collects 537 tests. The new test asserts rcParams are untouched across four entry modules, in a fresh interpreter each.
275eba1 to
63d347e
Compare
- `_sourcedata_files_for_subject`'s "known subjects" error message re-derived the manifest entries inline, without the isinstance(dict) guard that `_manifest_subject_files` applies. On a manifest with a non-dict entry the helper returns [], control reaches that raise, and building the error message itself raised AttributeError -- a crash inside the error path. Named above the raise, with the same guard. - `_is_stored_file` drops from 13 lines to 2: the try/except around `relative_to` was a long way of saying "the name is not absolute". Verified identical on a.txt, sub/, missing, /etc/hosts, "", ".", ../a.txt and sub/../a.txt. - Drops the `Returns` section from `nemar_sourcedata_is_local`; it restated the summary line, and this helper is not published in api.rst. `Parameters` stays: the `target_dir` entry documents why base.py hoists nemar_store() out of the per-subject loop. - The import side-effect test asserted `callable(moabb.benchmark)`, which is true of any eager import and tests nothing here, and pointed its AttributeError case at `moabb`, which has no __getattr__ -- so it got the interpreter's own message and never reached the branch this PR adds. Now aimed at `moabb.analysis`.
The deferred import silently removed an attribute. The eager `from moabb.analysis import plotting as plt` used to bind `plotting` on the package, so `import moabb.analysis; moabb.analysis.plotting` worked without importing the submodule. After deferring it, that raised AttributeError -- and the changelog claimed "every public name still resolves", which was not true. `__getattr__` now serves it, via importlib.import_module: `from . import plotting` falls back to getattr on this package and re-enters __getattr__ forever. Covered by a test. Also: - bnci_2025 declared `montage=` as a 213-character electrode list, which bids_interface writes verbatim into every BIDS sidecar's EEGPlacementScheme. Set to the montage the loader actually applies. The electrode names were already in `sensors`. - The `_sourcedata_store_holds_data` docstring justified os.walk with a syscall-count claim that measurement contradicts (rglob is faster there, because it stops on the first *directory*). That is exactly why rglob was wrong: an interrupted fetch leaving empty sub-* directories counted as a fetched store. Docstring now says that instead.
…nnels
The four GuttmannFlury2025 classes declared `"stim": 1` in `channel_types`.
That was harmless while `n_channels` was stored independently, but now that it
is `sum(channel_types.values())` it makes the four report 66 recorded channels
where 65 were recorded -- MOABB adds the stim channel itself, as
`test_metadata_matches_raw_data` says in the comment above the
`raw_counts.pop("stim", None)` it does before comparing.
That test is `@pytest.mark.download`, and NeuroTechXgh-1161 is the change that repairs
the monthly download job (it pointed at a file that does not exist and
collected nothing), so merging both would have surfaced this as an off-by-one
on four datasets.
…ounds MOABB 1.6.1 fixes both of these in the loader (NeuroTechX/moabb#1161): - `Srisrisawang2024Simultaneous._load_raw` retyped seven kinematic channels (hand position/velocity, the reaching target, a validity flag) to misc. `read_raw_eeglab` had typed them eeg, so every paradigm picking eeg fed the target position -- the label -- to the classifier. MOABB now types them in `_load_data_001_2025`, and names the EOG channels explicitly so the declared channel_types is true by construction rather than by assumption. - `Cattan2019Passive._load_raw` renamed Fc5/Fc6 to FC5/FC6 and applied standard_1020. MOABB now spells them correctly in `_chnames` and calls `set_montage` itself -- the montage :gh:`700` announced and never shipped. With the names corrected upstream the local rename resolves empty and `set_montage` is idempotent, so the override is a no-op. Both now resolve to `_BaseMoabb._load_raw`.
Four dataset-level defects found while auditing what downstream consumers have to work around. The first one changes reported results.
🔴
BNCI2025_001feeds the labels to the classifier_load_data_001_2025reads the EEGLAB file and never callsset_channel_types:The
.setfiles carry seven non-EEG channels next to the montage —x,y,vx,vy(hand kinematics),targetPosX,targetPoxY(the reaching target) andvalidity.read_raw_eeglabtypes anything it cannot recognise aseeg, so:METADATAclaimedn_channels=67— the difference is exactly those 7pick_channels_for_modalitiesdefaults tomne.pick_types(info, eeg=True, stim=False), so all 67 are picked as EEGThis is label leakage: any benchmark run on
BNCI2025_001today is inflated. The sibling loader in the same module,_convert_run_002_2025, already builds an explicitch_typeslist (["eeg"]*60 + ["eog"]*4), so this looks like an oversight rather than a decision.Results on this dataset will get worse, and that is the fix. Cached BIDS artifacts for it should be invalidated, since channel types are part of the cached raw. Anyone who genuinely wants the kinematics can still ask via
return_all_modalities=True, which becomes meaningful for this dataset instead of a no-op.Metadata is corrected alongside:
n_channels=71withchannel_types={"eeg": 60, "eog": 4, "misc": 7}, which now sums correctly and matches whattest_n_channels_matches_raw_datacounts (all non-stimchannels).miscinchannel_typesfollows the existing precedent inssvep_kim2025.py.🟠 Two datasets never got the montage gh-700 announced
The changelog entry for gh-700 reads "Fixing unit issue and lack of montage with … Rodrigues2017 … Cattan2019_PHMD …" (five datasets in total). The commit changed exactly one line per file —
S→S * 1e-6. ForCattan2019_PHMDandRodrigues2017there is noset_montagecall anywhere in their loaders (grep count: 0 in both), so the montage half was never implemented.It could not have worked as written either. Both share the same 16-electrode setup and both spell the two frontal-central electrodes
Fc5/Fc6— the only two of their sixteen names thatstandard_1020cannot resolve (FC5/FC6both do). Both are fixed here, in the channel lists and in theMETADATAsensor lists, and the montage is applied withon_missing="ignore", matching the house pattern inbnci/utils.py::make_rawandstieger2021.py.Rodrigues2017additionally declaredmontage="standard_1010"in itsMETADATA— not a montage MNE can build (make_standard_montagerejects it), so it would raise for anyone who took it at face value. Corrected tostandard_1020, which is what the 16 channels actually are.I audited the rest of that gh-700 list:
Huebner2018,BaseCastillos2023andOfner2017do set a montage.Cattan2019_VRis the third copy and is fixed too. All three are the same g.tec 16-electrode setup with the sameFc5/Fc6spelling. Fixing only two would have left MOABB shipping two spellings of one electrode:channels=["FC5", "FC6", "Cz"]would select onRodrigues2017/Cattan2019_PHMDand trip thelen(picks) == len(self.channels)assert onCattan2019_VR, and a benchmark spanning them would lose both electrodes to the channel intersection — a worse state than the original consistent defect. Its montage is now set once on the sharedInfo, so theif not ds.code == "Cattan2019-VR"branch (which exists for VR block splitting) no longer doubles as the reason its Raws had no positions.🟠
Cattan2019_PHMDannotates 60-second blocks as lasting 1 secondinterval=[0, 1], while the dataset's own metadata recordsblock_duration_s=60.0and its methodology says "Each block consisted of 1 minute of EEG recording".SetRawAnnotationsderives annotation durations frominterval, so every relaxation block was stamped 1 s long. Among the resting-state datasets this was the outlier —alphawavesuses[0, 10]for 10 s blocks,hinss2021[0, 2]for 2 s.Backward compatibility, traced through every consumer of
dataset.interval:paradigms/base.py:627usesinterval[0], unchanged at0→ annotation onsets do not moveparadigms/base.py:629usesinterval[1]only whenself.tmax is None;RestingStateToP300Adaptersetstmax=50, so its default path and the shipped example are unaffectedpreprocessing.py:783takes the stim-channel branch here, which ignoresinterval🟠 The NEMAR provider pin silently doesn't hold
_prefetch_nemar_sourcedatareturned as soon as the store held anything:But
SOURCEDATA_PROVENANCE = "sourcedata/sourcedata_provenance.json"lands inside that store. So for a caller that loads one subject at a time —get_data(subjects=[s])in a loop, which is how corpus builders drive this — subject 1 prefetches, the store becomes non-empty, and every subsequent subject skips the prefetch and falls through to the upstream host viadata_dl. With the provider pinned to"nemar"that silently breaks the guarantee the pin advertises. Worse, if the manifest downloads but the subject fetch fails, the store is non-empty forever and prefetch is permanently disabled for that dataset.Each requested subject is now prefetched.
sourcedata_pathpassesforce_update=False, sonemar.download(..., trust_existing=True)does not re-transfer files already on disk — the cost is one listing call per subject rather than a re-download. That is the honest price of actually honouring the pin.Follow-ups filed, not fixed here
download-test.ymlcollects nothing, so every@pytest.mark.downloadtest runs in no CI job.test_metadata_matches_raw_datawould have caught theBNCI2025_001leak below; it has never executed.n_channels != sum(channel_types) != len(sensors), andvalidate_metadata_against_datasetis exported but never called.METADATA.montagevalues MNE cannot build, and 31 loaders declare a montage they never apply.import moabbmutates the caller's global matplotlib rcParams.Not included
An unrelated issue found in the same audit:
import moabbmutates the caller's global matplotlibrcParams(font.familysans-serif→serif,axes.gridFalse→True, spines off), becausemoabb/__init__.py→benchmark→moabb.analysis→analysis/plotting.py:39callsset_moabb_defaults()at import time. Fixing it properly means moving that call out of import scope and into the plotting entry points, which is a behaviour change for anyone relying onimport moabbstyling their figures — so it does not belong in a bug-fix batch. Happy to open it separately.Split out
The
AcquisitionMetadata.n_channelschange that was here moved to #1166 — it touched 93 files mechanically and drowned these fixes in review. This PR is now 13 files.Version
The batch is bug fixes only, so the next release is a patch:
__version__goes to1.6.1dev0and the open changelog section is renamed to match. The earlier1.7.0dev0assumed a minor release that this content does not justify.Verification
moabb/tests/test_metadata.py+moabb/tests/test_dataset_fixes.py: 257 passedpre-commit runclean on all touched filesCattan2019_PHMDandRodrigues2017: all 16 EEG channel names now resolve againststandard_1020;Cattan2019_PHMD.interval == [0, 60]BNCI2025_001:n_channels == sum(channel_types) == 71