Add MOABB Dataset Integration - #74
Conversation
milosobral
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Task.MOTOR_IMAGERY and Task.P300 enums; replaced interval-level stratified folds with trial-level and subject-level split utilities; introduced a MOABBPipeline base class; added two MOABB-based pipelines (Physionet MI and Brain Invaders P300); updated Kemp sleep pipeline to use new split API; expanded split tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pipeline as MOABBPipeline<br/>(subclass)
participant MOABB as MOABB<br/>Dataset/Paradigm
participant SplitUtil as Split<br/>Utilities
participant Storage as HDF5<br/>Storage
User->>Pipeline: process(manifest_item)
activate Pipeline
Pipeline->>MOABB: download(subject, session)
activate MOABB
MOABB-->>Pipeline: raw EEG data
deactivate MOABB
Pipeline->>Pipeline: _extract_continuous_eeg(raw)
Pipeline->>Pipeline: _extract_trials_from_raw(raw)
Pipeline->>SplitUtil: generate_trial_folds(trials, stratify_by)
activate SplitUtil
SplitUtil-->>Pipeline: folds (train/valid/test)
deactivate SplitUtil
alt subject_id provided
Pipeline->>SplitUtil: generate_subject_kfold_assignment(subject_id)
activate SplitUtil
SplitUtil-->>Pipeline: subject-level fold assignment
deactivate SplitUtil
end
Pipeline->>Pipeline: get_brainset_description()
Pipeline->>Storage: assemble Data object + write HDF5
activate Storage
Storage-->>Pipeline: persisted artifact
deactivate Storage
Pipeline-->>User: completion status
deactivate Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@brainsets/moabb_pipeline.py`:
- Around line 347-358: When info.get("meas_date") is None, avoid using
datetime.datetime.now() for recording_date; instead set a deterministic sentinel
(e.g., datetime.datetime(1970,1,1, tzinfo=datetime.timezone.utc)) and emit a
warning so the missing meas_date is explicit; update the code around
info.get("meas_date") to assign recording_date = sentinel and call
logging.warning(...) with session_id and self.task before returning the
SessionDescription(id=session_id, recording_date=recording_date,
task=self.task).
- Around line 441-444: store_path existence check assumes self.args is set and
accesses self.args.reprocess, which can raise AttributeError in tests or direct
instantiation; replace that access with a safe lookup such as using
getattr(getattr(self, "args", None), "reprocess", False) (or assign
safe_reprocess = getattr(getattr(self, "args", None), "reprocess", False) above)
and then use if store_path.exists() and not safe_reprocess: call
self.update_status("Skipped Processing") and return; reference symbols:
self.args, reprocess, store_path, self.processed_dir, and self.update_status.
🧹 Nitpick comments (5)
brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (1)
56-84: Consider usingOptional[str]for type annotation consistency.The
subject_idparameter defaults toNonebut is typed asstr, which violates PEP 484. This should beOptional[str]for type safety.Also note: this implementation uses
setattrto attach subject assignments (lines 81-82), while theschalk_wolpaw_physionet_2009pipeline uses**subject_assignmentsin theDataconstructor. Consider aligning the approaches for consistency across pipelines.♻️ Suggested type annotation fix
+from typing import Optional + ... - def _generate_splits(self, trials, subject_id: str = None): + def _generate_splits(self, trials, subject_id: Optional[str] = None):brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (1)
68-103: Consider usingOptional[str]for type annotation.Same as the BI2014a pipeline - the
subject_idparameter should be annotated asOptional[str]for PEP 484 compliance.The implementation correctly generates task-specific stratified splits for all three task configurations and adds subject-level assignments when applicable.
♻️ Suggested type annotation fix
+from typing import Optional + ... - def _generate_splits(self, trials, subject_id: str = None): + def _generate_splits(self, trials, subject_id: Optional[str] = None):brainsets/moabb_pipeline.py (3)
150-205: Double data fetch is inefficient.The
paradigm.get_data()is called twice - once withreturn_epochs=False(line 153) and again withreturn_epochs=True(line 191). This downloads/processes the data twice, which could be slow for large datasets.Consider fetching with
return_epochs=Trueonce and extracting the array data from epochs, or caching the result.♻️ Suggested optimization
- X, labels, meta = paradigm.get_data( - dataset=dataset, - subjects=[subject], - return_epochs=False, - ) - - # ... validation and filtering ... - - epochs, _, meta_epochs = paradigm.get_data( + # Fetch epochs once and extract arrays + epochs_list, labels, meta = paradigm.get_data( dataset=dataset, subjects=[subject], return_epochs=True, ) + + # Extract array data from epochs + # X can be derived from epochs_list after session filteringNote: The exact refactor depends on how MOABB structures the epoch data. You may need to concatenate epoch arrays after filtering.
207-235: Consider documenting or using MNE constants for channel type mapping.The magic numbers in
ch_type_map(lines 225-231) correspond to MNE channel kinds, but this isn't immediately obvious. Consider adding a comment referencing MNE's channel kind constants or importing them directly.
386-410: Unusedsubject_idparameter is an intentional extension point.The
subject_idparameter is unused in the base implementation but provides an extension point for subclasses to add subject-level splits. This is a valid pattern, though it would benefit from a brief comment explaining this design choice.The type annotation should use
Optional[str]per PEP 484.♻️ Suggested improvement
- def _generate_splits(self, trials, subject_id: str = None): + def _generate_splits(self, trials, subject_id: Optional[str] = None): """Generate stratified folds for trials. Parameters ---------- trials : Interval Trial intervals with label fields subject_id : str, optional - Subject identifier for subject-level splits. Default is None. + Subject identifier for subject-level splits. Unused in base class + but available for subclass extensions. Default is None.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
brainsets/moabb_pipeline.pybrainsets/taxonomy/task.pybrainsets/utils/split.pybrainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.pybrainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py
🧰 Additional context used
🧬 Code graph analysis (3)
brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (5)
brainsets/descriptions.py (1)
BrainsetDescription(13-40)brainsets/taxonomy/task.py (1)
Task(4-30)brainsets/moabb_pipeline.py (3)
MOABBPipeline(33-476)_generate_splits(386-410)get_brainset_description(302-310)brainsets/utils/split.py (1)
compute_subject_kfold_assignments(415-465)brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (3)
Pipeline(41-113)_generate_splits(68-103)get_brainset_description(105-113)
brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (3)
brainsets/descriptions.py (1)
BrainsetDescription(13-40)brainsets/taxonomy/task.py (1)
Task(4-30)brainsets/utils/split.py (2)
generate_task_kfold_splits(332-412)compute_subject_kfold_assignments(415-465)
brainsets/moabb_pipeline.py (5)
brainsets/pipeline.py (1)
BrainsetPipeline(13-184)brainsets/descriptions.py (4)
BrainsetDescription(13-40)SessionDescription(72-87)SubjectDescription(44-68)DeviceDescription(91-124)brainsets/taxonomy/subject.py (1)
Species(4-10)brainsets/taxonomy/task.py (1)
Task(4-30)brainsets/utils/split.py (1)
generate_stratified_folds(242-329)
🪛 Ruff (0.14.11)
brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py
44-44: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
56-56: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py
47-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
54-60: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
62-66: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
68-68: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
brainsets/utils/split.py
370-372: Avoid specifying long messages outside the exception class
(TRY003)
445-445: Probable use of insecure hash functions in hashlib: md5
(S324)
457-457: Probable use of insecure hash functions in hashlib: md5
(S324)
brainsets/moabb_pipeline.py
62-62: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
63-63: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
84-84: Unused class method argument: raw_dir
(ARG003)
84-84: Unused class method argument: args
(ARG003)
157-159: Avoid specifying long messages outside the exception class
(TRY003)
166-169: Avoid specifying long messages outside the exception class
(TRY003)
173-176: Avoid specifying long messages outside the exception class
(TRY003)
180-182: Avoid specifying long messages outside the exception class
(TRY003)
297-297: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
376-376: Avoid specifying long messages outside the exception class
(TRY003)
386-386: Unused method argument: subject_id
(ARG002)
386-386: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dandi-testing
🔇 Additional comments (15)
brainsets/utils/split.py (3)
2-5: LGTM!The new imports are appropriate for the added functionality:
hashlibfor deterministic hashing,Dictfor type hints, andCounterfor label distribution logging.
332-412: Well-structured task-specific k-fold split generation.The function correctly validates inputs, filters trials per task, handles edge cases with insufficient trials via warning, and leverages the existing
generate_stratified_foldsutility. The logging of per-fold label distributions is helpful for debugging stratification quality.
443-463: MD5 usage is acceptable for deterministic assignment.The static analysis warning about MD5 (S324) is a false positive in this context. MD5 is being used for deterministic hashing to assign subjects to folds reproducibly, not for cryptographic security. This is a valid use case.
One minor observation: the hash resolution at line 459 (
fold_hash_int % 10000) provides 10,000 discrete values, which is sufficient for typical dataset sizes but could introduce slight quantization bias for very preciseval_ratiovalues. This is unlikely to matter in practice.brainsets/taxonomy/task.py (1)
26-31: LGTM!Clean addition of
MOTOR_IMAGERYandP300task types with appropriate sequential values (8 and 9) and descriptive comments. These align well with the new MOABB dataset integrations.brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (3)
1-8: LGTM!The PEP 723 inline script metadata correctly specifies the Python version and runtime dependencies (
mne,moabb,scikit-learn) needed for this pipeline.
38-54: LGTM!The pipeline configuration is well-structured with clear mappings for the P300 paradigm. The
label_mapcorrectly encodes Target=1 and NonTarget=0, which is standard for P300 binary classification.The static analysis warning about mutable class attributes (RUF012) is low-risk here since these are read-only configuration values not mutated at runtime.
86-95: LGTM!The brainset description provides appropriate metadata with a clear description of the BI2014a P300 dataset and link to the MOABB documentation.
brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (3)
1-8: LGTM!The PEP 723 inline script metadata correctly specifies dependencies consistent with the BI2014a pipeline.
41-66: Well-designed task configuration.The
TASK_CONFIGSdictionary enables flexible experimentation with different motor imagery classification tasks (full 5-class, left/right binary, right-hand/feet binary). Thedataset_kwargscorrectly filters to imagined-only movements per the PR description.
105-113: LGTM!The brainset description provides appropriate metadata with a clear description of the PhysioNet Motor Imagery dataset.
brainsets/moabb_pipeline.py (5)
1-31: LGTM!The module docstring clearly explains the purpose, and the imports are appropriate for the MOABB integration functionality.
33-71: Comprehensive base class design.The class docstring clearly documents the contract for subclasses, including required attributes and methods. The separation of concerns between base class (download/process workflow) and subclass (brainset-specific configuration) is well-designed.
The mutable class attributes (
dataset_kwargs,paradigm_kwargs) flagged by static analysis are low-risk since they're read-only configuration defaults.
237-299: LGTM!The EEG extraction logic correctly:
- Concatenates epoched data into a continuous signal
- Computes trial boundaries from sample counts
- Maps labels to integer IDs via
label_map- Validates that trials don't overlap
The domain calculation and timestamp computation (midpoint of each trial) are appropriate.
459-476: LGTM!The final Data object assembly and HDF5 serialization are well-structured. The dynamic
trial_keyattribute assignment (line 467) correctly uses the subclass-defined key name.
102-117: The review comment is incorrect. MOABB datasets are guaranteed to have integer subject IDs.MOABB's
BaseDatasetAPI explicitly specifies thatsubject_listcontains integers (List[int]), not strings. The code at line 108 correctly uses the:03dformat specifier for integer subjects, and no type mismatch risk exists. The concern about non-integer subject IDs is not applicable to MOABB datasets.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Can we please talk? discord aristimunha |
There was a problem hiding this comment.
Hey Milo,
I went through the PR and I left some comments. Two things I'm not sure about:
-
Having a class for MOABB utils givent hat the other utils only have functions, does not seem consistent. Is there any particular advantage of having a class in this case?
-
This version of MOABB does not return all the trials at least for physionet. Not sure what the best solution to this is, it might not even be a problem. We can discuss this.
One last thing that I think might be important to maybe add to the docstrings in the pipeline.py script is to specify which preprocessing steps the original data has gone through. I think by default MOABB applies a bandpass filter between 8-32Hz, not sure if we can get rid of that; it also cuts the trials to 3secs and sometimes it also applies referencing (not sure when it does and when it does not).
The problem of using MOABB API's is that we are creating like a wrapper of a wrapper and we are not sure what they are doing with the data behind the scenes. One option that I considered at some point was to have the MOABB utils to get these datasets as MOABB if people want to compare to MOABB baselines, but maybe there should be a separate brainsets for Physionet that get the data from the original source.
oumgharbi
left a comment
oumgharbi
left a comment
There was a problem hiding this comment.
Thank your Milo for this PR. I added some minor comments.
While I agree and like the idea of having a base pipeline class for MOABB datasets i have one main concern. I am not sure about using and depending on the MOABB package.
Like Laura said, I feel like this adds an extra layer to an existing framework with potential duplications of existing abstractions in brainsets. Future users may need to learn both moabb and brainsets to use this base pipeline.
Also this new dependency may add maintenance overhead with potential compatibility issues.
I like that this introduces a new logic seperating datasets from task definition and process using paradigms, but I'm not sure how well it fits in the brainsets logic. Maybe it would be better to implement this in torch_brain? We could further discuss this. 🙂
|
Hey folks! I had first iteration with @milosobral at discord, and I can help with this if you want. I open to more iterations too! You can get the data without the paradigm object and without pre-processing. About the events selection, I would recommend to operate this with moabb api. Let's collaborate ✌️ |
|
I will comment within the PR soon |
…efactored trial slicing to keep all data. - Introduced command-line arguments for bandpass filter settings and resampling rate. - Implemented validation for bandpass parameters against Nyquist frequency. - Updated data retrieval methods to support new filtering options. - Refactored related pipelines to utilize the new base argument parser.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@brainsets/utils/moabb/pipeline.py`:
- Around line 537-561: The _generate_splits method currently accepts a
subject_id parameter but never uses it; remove subject_id from the signature and
docs (and any related references) if subject-level splitting isn't needed, or
implement subject-level logic by passing subject_id into generate_trial_folds
(or pre-filtering/grouping trials by subject) and adjust
stratify_by/self.stratify_field handling accordingly; update the docstring to
reflect the chosen behavior and keep generate_trial_folds, folds_dict, and
Data(**folds_dict, domain=trials) usage unchanged.
- Around line 527-535: The code in the DeviceDescription creation silently
substitutes a sentinel date when info.get("meas_date") is None; update the
method that builds the DeviceDescription (the block that defines recording_date
and returns DeviceDescription) to emit a warning when meas_date is missing (same
style/text as _get_session_description) including the subject_id and that a
sentinel date (2026-01-15) is being used; use the module/logger used elsewhere
in this file (or warnings.warn if consistent) so the warning appears in logs and
keep the DeviceDescription id creation unchanged.
- Around line 254-276: The code currently monkey-patches the private MOABB
method _get_raw_pipelines (in functions handling no_filtering/needs_resample)
and also lacks MOABB as a declared dependency; update pyproject.toml to add a
tested MOABB version constraint, and refactor the monkey-patch to use MOABB's
public APIs instead (e.g., construct FixedPipeline instances or use
paradigm.make_process_pipelines()/make_process_pipelines to return pipelines
composed from get_filter_pipeline and get_resample_pipeline or via
make_fixed_pipeline), replacing assignments to paradigm._get_raw_pipelines and
instead registering/returning pipelines through the documented public hooks;
also add a brief comment documenting which MOABB versions this behavior was
validated against.
🧹 Nitpick comments (3)
brainsets/utils/moabb/pipeline.py (3)
96-97: Consider annotating mutable class attributes withClassVar.Mutable class attributes like
{}can lead to unintended state sharing if mutated in-place rather than reassigned. While subclasses typically override these entirely, addingClassVarclarifies intent and prevents accidental mutation.♻️ Suggested improvement
+from typing import ClassVar ... - dataset_kwargs: Dict[str, Any] = {} - paradigm_kwargs: Dict[str, Any] = {} + dataset_kwargs: ClassVar[Dict[str, Any]] = {} + paradigm_kwargs: ClassVar[Dict[str, Any]] = {}
319-343: Consider using MNE's built-in channel type detection.The hard-coded FIFF constants are fragile and may not cover all channel types. MNE provides
mne.channel_type(info, idx)which handles this comprehensively.♻️ Suggested refactor using MNE API
def _get_channel_types(self, info): - # MNE channel kind constants (mne.io.constants.FIFF) - ch_type_map = { - 2: "EEG", # FIFFV_EEG_CH - 202: "EOG", # FIFFV_EOG_CH - 302: "EMG", # FIFFV_EMG_CH - 402: "ECG", # FIFFV_ECG_CH - 502: "MISC", # FIFFV_MISC_CH - } - return [ - ch_type_map.get(info["chs"][info["ch_names"].index(ch)]["kind"], "MISC") - for ch in info["ch_names"] - ] + return [mne.channel_type(info, idx).upper() for idx in range(len(info["ch_names"]))]
383-391: Unknown labels silently mapped to -1.Labels not found in
self.label_mapare silently assigned ID-1(line 391). This could mask configuration errors or unexpected events in the data.Consider logging a warning when unknown labels are encountered.
🔧 Suggested improvement
- id_values = np.array([self.label_map.get(label, -1) for label in labels]) + id_values = [] + for label in labels: + id_val = self.label_map.get(label, -1) + if id_val == -1: + logging.warning(f"Unknown label '{label}' mapped to ID -1") + id_values.append(id_val) + id_values = np.array(id_values)
…tter dataset identification
Updated the Korczowski and Schalk pipelines to include their respective dataset_sign values ("BRAININVADERS2014A" and "EEGBCI").
estefanysuarez
left a comment
estefanysuarez
left a comment
There was a problem hiding this comment.
Hey Milo!
Thanks for addressing the comments. This is what I noticed:
- Trials are no longer 3secs, which is good cause they are not cropped anymore
- I plot the PSD of the signal and it seems the bandpass filtering is effectively being removed
- My major comment would be in the timestamps of the trials (see details in
brainsets/utils/moabb/pipeline.py). Maybe this is just a problem with Physionet and the fact that their API only returns 174 out of 180 trials (not sure why?). I didn't try with others, but I believe this should be fixed before merging. One thing I can tell you though, we were using this code to obtain the trials and I believe this was wrking correctly. Notice that the big difference here is in the definition of epochs and how single subject data is being obtained:
def extract_eeg_data(recording_data) -> RegularTimeSeries:
sfreq = recording_data["0"].info["sfreq"]
num_epochs = len(recording_data)
epoch_data = [recording_data[str(i)].get_data().T for i in range(num_epochs)]
eeg_signals = np.concatenate(epoch_data)
# make regular time series
eeg = RegularTimeSeries(
signal=eeg_signals,
sampling_rate=sfreq,
domain=Interval(
start=np.array([0.0]),
end=np.array([(len(eeg_signals) - 1) / sfreq]),
),
)
units = ArrayDict(
id=np.array(recording_data["0"].ch_names, dtype="U"),
types=np.array(recording_data["0"].get_channel_types(), dtype="U"),
)
epoch_start = [0.0]
epoch_end = []
for m in epoch_data:
if len(epoch_end) > 0:
epoch_start.append(epoch_end[-1])
epoch_end.append(epoch_start[-1] + len(m) / sfreq)
epochs = Interval(
start=np.array(epoch_start),
end=np.array(epoch_end),
)
return eeg, units, epochs
def extract_motor_imagery_trials(recording_data, epochs) -> Interval:
start_times = []
end_times = []
movements = []
for i in range(len(epochs)):
epoch_start = epochs.start[i]
for annotation in recording_data[str(i)].annotations:
start_time = annotation["onset"] + epoch_start
end_time = start_time + annotation["duration"]
if len(end_times) > 0 and start_time < end_times[-1]:
# the previous trial goes into the next one
# this happens because of numerical precision issues
assert (
end_times[-1] - start_time < 0.1
), f"found overlap between trials: start time of trial i: {start_time}, end time of trial i-1: {end_times[-1]}"
# we can clip the end time of the last trial
end_times[-1] = start_time
start_times.append(start_time)
end_times.append(end_time)
movements.append(annotation["description"])
trials = Interval(
start=np.array(start_times),
end=np.array(end_times),
timestamps=(np.array(start_times) + np.array(end_times)) / 2,
movements=np.array(movements),
movement_ids=np.array([MOVEMENT_ID_MAP[m] for m in movements]),
timekeys=["start", "end", "timestamps"],
)
if not trials.is_disjoint():
raise ValueError("Found overlapping trials")
return trials
Where data is obtained in the following way:
dataset = PhysionetMI(imagined=True, executed=False)
# load data
dataset.get_data(subjects=[args.subject])
recording_data = dataset._get_single_subject_data(args.subject)["0"]
| for ch in info["ch_names"] | ||
| ] | ||
|
|
||
| def _extract_trials_from_raw(self, raw, dataset) -> Interval: |
There was a problem hiding this comment.
I was looking at the start and end timestamps of each trial and I noticed two things:
- One good thing is that trials are not cropped to 3secs anymore
- On the downside, because of the issue I mentioned in my previous review, MOABB API only returns 174 out of 180 trials, making the duration of some of the trials incorrect. For instance, for subject 1, the first 10 trials are ~4.1-4.2 secs, as shown below:
Trial 0: Start: 4.2, End: 8.3 - Duration: 4.1000000000000005 - Trial: right_hand
Trial 1: Start: 8.3, End: 12.5 - Duration: 4.199999999999999 - Trial: rest
Trial 2: Start: 12.5, End: 16.6 - Duration: 4.100000000000001 - Trial: left_hand
Trial 3: Start: 16.6, End: 20.8 - Duration: 4.199999999999999 - Trial: rest
Trial 4: Start: 20.8, End: 24.9 - Duration: 4.099999999999998 - Trial: left_hand
Trial 5: Start: 24.9, End: 29.1 - Duration: 4.200000000000003 - Trial: rest
Trial 6: Start: 29.1, End: 33.2 - Duration: 4.100000000000001 - Trial: right_hand
Trial 7: Start: 33.2, End: 37.4 - Duration: 4.199999999999996 - Trial: rest
Trial 8: Start: 37.4, End: 41.5 - Duration: 4.100000000000001 - Trial: right_hand
Trial 9: Start: 41.5, End: 45.7 - Duration: 4.200000000000003 - Trial: rest
Trial 10: Start: 45.7, End: 49.8 - Duration: 4.099999999999994 - Trial: left_hand
But there is a few that are longer and it is because for some reason there are 6 missing 'rest' trials:
Trial 27: Start: 116.2, End: 120.4 - Duration: 4.200000000000003 - Trial: rest
Trial 28: Start: 120.4, End: 129.2 - Duration: 8.799999999999983 - Trial: left_hand
Trial 29: Start: 129.2, End: 133.3 - Duration: 4.100000000000023 - Trial: left_hand
Another example:
Trial 56: Start: 241.2, End: 245.4 - Duration: 4.200000000000017 - Trial: rest
Trial 57: Start: 245.4, End: 254.2 - Duration: 8.799999999999983 - Trial: left_hand
Trial 58: Start: 254.2, End: 258.3 - Duration: 4.100000000000023 - Trial: right_hand
As you can see, rest and hands alternate, but in those two segments, there is a rest trial missing.
| splits : Data | ||
| Data object containing fold splits | ||
| """ | ||
| folds = generate_trial_folds( |
There was a problem hiding this comment.
I believe this is across all trials, right?
|
|
||
| logging.basicConfig(level=logging.INFO) | ||
|
|
||
| parser = ArgumentParser(parents=[_base_parser]) |
There was a problem hiding this comment.
_base_parser should not have an underscore at the start, since it's intended to be a public object.
| Subclasses must define: | ||
| - brainset_id: str | ||
| - dataset_class: Type[BaseDataset] | ||
| - paradigm_class: Type[BaseParadigm] | ||
| - dataset_sign: str (e.g., "EEGBCI", "BRAININVADERS2014A") | ||
| - dataset_kwargs: Dict[str, Any] (optional, defaults to {}) | ||
| - paradigm_kwargs: Dict[str, Any] (optional, defaults to {}) | ||
| - task: Task (e.g., Task.MOTOR_IMAGERY, Task.P300) | ||
| - trial_key: str (e.g., "motor_imagery_trials", "p300_trials") | ||
| - label_field: str (e.g., "movements", "targets") | ||
| - id_field: str (e.g., "movement_ids", "target_ids") | ||
| - stratify_field: str (e.g., "movements", "targets") | ||
| - label_map: Dict[str, int] (mapping from label strings to integer IDs) |
There was a problem hiding this comment.
Could this be validated in the pipeline's __init__?
| def _generate_splits(self, trials, subject_id: str = None): | ||
| """Generate stratified folds and subject-level k-fold assignments. | ||
|
|
There was a problem hiding this comment.
Same comment here: Since this method is supposed to be overridden publicly, it should not start with an underscore.
| Subclasses must implement: | ||
| - get_brainset_description(): Return dataset-specific BrainsetDescription | ||
|
|
There was a problem hiding this comment.
Does BrainsetDescription need to be programmatically generated? If not, then it could be made into a constant class attribute like the other attributes.
| @@ -237,39 +239,74 @@ def _create_interval_split(intervals: Interval, indices: np.ndarray) -> Interval | |||
| return split | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I strongly suggest using the splits from the moabb itself; a small modification within the partition will change the results and replicability.
References from my thesis that discuss this:
Del Pup, F., Zanola, A., Tshimanga, L. F., Bertoldo, A., Finos, L., & Atzori, M. (2025). The role of data partitioning on the performance of EEG-based deep learning models in supervised cross-subject analysis: a preliminary study. Computers in Biology and Medicine, 196, 110608.
Wosiak, A., Sumiński, M., & Żykwińska, K. (2025). Impact of Temporal Window Shift on EEG-Based Machine Learning Models for Cognitive Fatigue Detection. Algorithms, 18(10), 629.
Racz, F. S., & Csukly, G. (2025). Information Leakage and Performance Overestimation in EEG-Based Schizophrenia Detection: Evidence from Literature and Empirical Analyses. medRxiv, 2025-12.
Carlson, D. E., Chavarriaga, R., Liu, Y., Lotte, F., & Lu, B. L. (2025). The NERVE-ML (neural engineering reproducibility and validity essentials for machine learning) checklist: ensuring machine learning advances neural engineering. Journal of Neural Engineering, 22(2), 021002.
Varoquaux, G. (2018). Cross-validation failure: Small sample sizes lead to large error bars. Neuroimage, 180, 68-77.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py (1)
76-76:subject_id: str = Noneshould beOptional[str] = None.Same implicit
Optional(RUF013) as in the korczowski pipeline.♻️ Proposed fix
- def generate_splits(self, trials, subject_id: str = None): + def generate_splits(self, trials, subject_id: Optional[str] = None):Also add
Optionalto the imports at the top of the file:+from typing import Optional🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py` at line 76, The function signature generate_splits(self, trials, subject_id: str = None) should use Optional for clarity and type-checking; change it to subject_id: Optional[str] = None and add Optional to the typing imports at the top of the file (ensure Optional is imported alongside any existing typing imports). Update the function definition for generate_splits and the import statement to reference Optional so static checkers no longer flag the implicit optional type.brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py (1)
63-90: UseOptional[str]instead of implicitstr = None, and prefer passing subject assignments through theDataconstructor.Two nits:
subject_id: str = Noneis PEP 484-invalid implicitOptional(Ruff RUF013).- The physionet pipeline composes the
Dataobject once viaData(**task_splits, **subject_assignments, domain=trials). This pipeline instead callssuper()then mutates the result withsetattr, which is inconsistent and fragile iftemporaldata.Dataever adds__slots__or a frozen mode.♻️ Proposed fix
- def generate_splits(self, trials, subject_id: str = None): + def generate_splits(self, trials, subject_id: Optional[str] = None): ... splits = super().generate_splits(trials, subject_id=subject_id) if subject_id is not None: subject_assignments = generate_subject_kfold_assignment( subject_id, n_folds=3, val_ratio=0.2, seed=42 ) - for key, value in subject_assignments.items(): - setattr(splits, key, value) + # Re-build to include subject assignments in a single constructor call + folds_dict = {f"fold_{i}": getattr(splits, f"fold_{i}") for i in range(3)} + splits = Data(**folds_dict, **subject_assignments, domain=trials) return splits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py` around lines 63 - 90, The generate_splits method uses an implicit Optional by declaring subject_id: str = None and mutates the returned splits with setattr which is fragile; change the signature to subject_id: Optional[str] and, after calling super().generate_splits(...), merge subject_assignments into the Data object by constructing a new Data from combined dicts instead of mutating splits (i.e., call generate_subject_kfold_assignment(...) when subject_id is not None, then create/return Data(**splits.__dict__ or splits.as_dict(), **subject_assignments, domain=trials) or the equivalent Data constructor used elsewhere to keep composition consistent and avoid setattr/frozen/slots issues).brainsets/utils/moabb/pipeline.py (2)
90-91: Mutabledictclass attributes should be annotated withClassVaror usefield(default_factory=...).
dataset_kwargsandparadigm_kwargsare shared mutable dicts at the class level. If any code path ever mutates them (e.g.,cls.dataset_kwargs["key"] = val), the mutation leaks across all subclasses that don't override them.♻️ Proposed fix
+from typing import ClassVar ... - dataset_kwargs: Dict[str, Any] = {} - paradigm_kwargs: Dict[str, Any] = {} + dataset_kwargs: ClassVar[Dict[str, Any]] = {} + paradigm_kwargs: ClassVar[Dict[str, Any]] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/moabb/pipeline.py` around lines 90 - 91, dataset_kwargs and paradigm_kwargs are currently mutable dicts defined at class level which can leak mutations across subclasses; either mark them as class-level constants by annotating them as ClassVar[Dict[str, Any]] (e.g., dataset_kwargs: ClassVar[Dict[str, Any]] = {}) if you intend them to be shared and not mutated per-instance, or make them per-instance dataclass fields using field(default_factory=dict) (e.g., dataset_kwargs: Dict[str, Any] = field(default_factory=dict)) so each instance gets its own dict; update the declarations for dataset_kwargs and paradigm_kwargs accordingly in pipeline.py and adjust any code that mutates them to use instance attributes if you choose the default_factory approach.
355-356: Replace the privatemne.utils._get_stim_channelwith the publicmne.pick_typesAPI to avoid tight coupling to MNE internals.The private function exists in MNE 1.11.0, but using it couples the code to internal implementation details. Since the code only checks whether stim channels exist (not their names), the public API achieves the same result.
♻️ Suggested refactor
- stim_channels = mne.utils._get_stim_channel(None, raw.info, raise_error=False) - if len(stim_channels) > 0: + stim_indices = mne.pick_types(raw.info, meg=False, eeg=False, stim=True) + if len(stim_indices) > 0:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brainsets/utils/moabb/pipeline.py` around lines 355 - 356, Replace the private call to mne.utils._get_stim_channel by using the public picker: call mne.pick_types(raw.info, stim=True) (e.g., assign to stim_picks or reuse stim_channels name) and then check if len(stim_picks) > 0; this removes the dependency on the internal mne.utils._get_stim_channel while preserving the logic that detects presence of stim channels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brainsets/utils/moabb/pipeline.py`:
- Around line 264-268: recording_data keys are being sorted lexicographically
causing wrong session mapping; change how session_values is computed in the
block using dataset._get_single_subject_data/recording_data and session to sort
keys numerically when they contain numeric indices (e.g., "session_0",
"session_10")—implement a sort key that extracts the integer part (fallback to
original string if no integer found) so session_values =
sorted(recording_data.keys(), key=<extract-int-then-str>) and then continue to
pick session_key = session_values[session]; this preserves behavior for
non-numeric keys while ensuring correct index mapping for numbered session
names.
- Around line 386-393: The event time computation uses raw sample indices
directly so it ignores raw.first_samp; change the math to account for the
raw.offset by using (events[:, 0] - raw.first_samp) / sfreq when computing
starts (and similarly for ends entries derived from events) and compute the
final end time using (raw.first_samp + raw.n_times - 1) / sfreq instead of
raw.n_times - 1, so update references to starts, ends, events, sfreq,
raw.first_samp, last_sample and raw.n_times accordingly.
- Around line 183-185: The method _validate_bandpass_params accesses
self.args.bandpass_high directly which can raise AttributeError if self.args is
None; change the access to use getattr(self, "args", None) or getattr(self.args,
"bandpass_high", None) similarly to the defensive access used in process(),
e.g., replace direct uses of self.args.bandpass_high (and any other self.args.X)
inside _validate_bandpass_params with a safe getattr pattern so the method
handles a missing self.args without crashing.
In `@brainsets/utils/split.py`:
- Around line 541-554: The MD5 calls using hashlib.md5(subject_bytes) and
hashlib.md5(fold_bytes) should be changed to include the usedforsecurity flag to
avoid FIPS failures: replace hashlib.md5(subject_bytes) with
hashlib.md5(subject_bytes, usedforsecurity=False) and replace
hashlib.md5(fold_bytes) with hashlib.md5(fold_bytes, usedforsecurity=False);
update the variables hash_obj and fold_hash_obj accordingly so the subsequent
hexdigest()/int(...) logic remains the same.
---
Duplicate comments:
In `@brainsets/utils/moabb/pipeline.py`:
- Around line 538-562: The subject_id parameter of generate_splits is
intentionally unused but triggers static-analysis ARG002; update the
generate_splits method to make this explicit by either adding a "# noqa: ARG002"
comment next to the function signature or adding a no-op pass-through that
references subject_id (e.g., a short debug/log statement like
logging.debug("generate_splits subject_id=%s", subject_id)) so the linter
understands the parameter is intentionally retained; locate the method named
generate_splits in the class in pipeline.py and apply one of these two fixes
consistently.
- Around line 528-532: In _get_device_description, detect when
info.get("meas_date") returns None and before substituting the sentinel datetime
(currently assigned to recording_date), emit a warning via the same logger used
in _get_session_description indicating missing meas_date/metadata; specifically
update the branch that sets recording_date = datetime.datetime(2026, 1, 15,
tzinfo=datetime.timezone.utc) to first call logger.warning with a clear message
(e.g., missing meas_date for this recording) so absence of real recording_date
is logged consistently with _get_session_description.
---
Nitpick comments:
In `@brainsets_pipelines/korczowski_brain_invaders_2014a/pipeline.py`:
- Around line 63-90: The generate_splits method uses an implicit Optional by
declaring subject_id: str = None and mutates the returned splits with setattr
which is fragile; change the signature to subject_id: Optional[str] and, after
calling super().generate_splits(...), merge subject_assignments into the Data
object by constructing a new Data from combined dicts instead of mutating splits
(i.e., call generate_subject_kfold_assignment(...) when subject_id is not None,
then create/return Data(**splits.__dict__ or splits.as_dict(),
**subject_assignments, domain=trials) or the equivalent Data constructor used
elsewhere to keep composition consistent and avoid setattr/frozen/slots issues).
In `@brainsets_pipelines/schalk_wolpaw_physionet_2009/pipeline.py`:
- Line 76: The function signature generate_splits(self, trials, subject_id: str
= None) should use Optional for clarity and type-checking; change it to
subject_id: Optional[str] = None and add Optional to the typing imports at the
top of the file (ensure Optional is imported alongside any existing typing
imports). Update the function definition for generate_splits and the import
statement to reference Optional so static checkers no longer flag the implicit
optional type.
In `@brainsets/utils/moabb/pipeline.py`:
- Around line 90-91: dataset_kwargs and paradigm_kwargs are currently mutable
dicts defined at class level which can leak mutations across subclasses; either
mark them as class-level constants by annotating them as ClassVar[Dict[str,
Any]] (e.g., dataset_kwargs: ClassVar[Dict[str, Any]] = {}) if you intend them
to be shared and not mutated per-instance, or make them per-instance dataclass
fields using field(default_factory=dict) (e.g., dataset_kwargs: Dict[str, Any] =
field(default_factory=dict)) so each instance gets its own dict; update the
declarations for dataset_kwargs and paradigm_kwargs accordingly in pipeline.py
and adjust any code that mutates them to use instance attributes if you choose
the default_factory approach.
- Around line 355-356: Replace the private call to mne.utils._get_stim_channel
by using the public picker: call mne.pick_types(raw.info, stim=True) (e.g.,
assign to stim_picks or reuse stim_channels name) and then check if
len(stim_picks) > 0; this removes the dependency on the internal
mne.utils._get_stim_channel while preserving the logic that detects presence of
stim channels.
| """ | ||
| fmax = self.args.bandpass_high | ||
| if fmax is None: |
There was a problem hiding this comment.
self.args accessed without a defensive guard in _validate_bandpass_params.
Line 184 reads self.args.bandpass_high directly, which raises AttributeError if self.args is None. The same pattern was already fixed defensively in process() (using getattr), but the fix was not applied here.
🔧 Proposed fix
- fmax = self.args.bandpass_high
+ fmax = getattr(getattr(self, "args", None), "bandpass_high", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets/utils/moabb/pipeline.py` around lines 183 - 185, The method
_validate_bandpass_params accesses self.args.bandpass_high directly which can
raise AttributeError if self.args is None; change the access to use
getattr(self, "args", None) or getattr(self.args, "bandpass_high", None)
similarly to the defensive access used in process(), e.g., replace direct uses
of self.args.bandpass_high (and any other self.args.X) inside
_validate_bandpass_params with a safe getattr pattern so the method handles a
missing self.args without crashing.
| recording_data = dataset._get_single_subject_data(subject) | ||
|
|
||
| session_values = sorted(recording_data.keys()) | ||
| if session < len(session_values): | ||
| session_key = session_values[session] |
There was a problem hiding this comment.
Lexicographic session-key sort may produce incorrect index mapping for multi-session datasets.
sorted(recording_data.keys()) on string session names (e.g., "session_0", "session_1", ..., "session_10") uses lexicographic order, which would place "session_10" before "session_2". The manifest generates sessions as integer indices 0, 1, 2, ..., so a mismatch silently maps the wrong session to a manifest row. For the two datasets in this PR (PhysionetMI with 1 session, BI2014a with ≤3) this is benign, but the base class is designed for general reuse.
🔧 Proposed fix — sort numerically when possible
- session_values = sorted(recording_data.keys())
+ try:
+ session_values = sorted(recording_data.keys(), key=lambda s: int(s.split("_")[-1]))
+ except (ValueError, IndexError):
+ session_values = sorted(recording_data.keys())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| recording_data = dataset._get_single_subject_data(subject) | |
| session_values = sorted(recording_data.keys()) | |
| if session < len(session_values): | |
| session_key = session_values[session] | |
| recording_data = dataset._get_single_subject_data(subject) | |
| try: | |
| session_values = sorted(recording_data.keys(), key=lambda s: int(s.split("_")[-1])) | |
| except (ValueError, IndexError): | |
| session_values = sorted(recording_data.keys()) | |
| if session < len(session_values): | |
| session_key = session_values[session] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets/utils/moabb/pipeline.py` around lines 264 - 268, recording_data
keys are being sorted lexicographically causing wrong session mapping; change
how session_values is computed in the block using
dataset._get_single_subject_data/recording_data and session to sort keys
numerically when they contain numeric indices (e.g., "session_0",
"session_10")—implement a sort key that extracts the integer part (fallback to
original string if no integer found) so session_values =
sorted(recording_data.keys(), key=<extract-int-then-str>) and then continue to
pick session_key = session_values[session]; this preserves behavior for
non-numeric keys while ensuring correct index mapping for numbered session
names.
| starts = events[:, 0] / sfreq | ||
|
|
||
| ends = np.zeros(len(events)) | ||
| for i in range(len(events) - 1): | ||
| ends[i] = events[i + 1, 0] / sfreq | ||
|
|
||
| last_sample = raw.n_times - 1 | ||
| ends[-1] = last_sample / sfreq |
There was a problem hiding this comment.
Event timestamps don't account for raw.first_samp offset.
events[:, 0] from both mne.find_events and mne.events_from_annotations are absolute sample numbers (i.e., raw.first_samp + n). Dividing directly by sfreq gives correct times only when raw.first_samp == 0, which holds for concatenate_raws output and most MOABB EDF files, but not in general. The same offset should be applied to the last-sample calculation at line 392.
🔧 Proposed fix
- starts = events[:, 0] / sfreq
+ starts = (events[:, 0] - raw.first_samp) / sfreq
ends = np.zeros(len(events))
for i in range(len(events) - 1):
- ends[i] = events[i + 1, 0] / sfreq
+ ends[i] = (events[i + 1, 0] - raw.first_samp) / sfreq
last_sample = raw.n_times - 1
- ends[-1] = last_sample / sfreq
+ ends[-1] = last_sample / sfreq # raw.times[-1], always relative to first_samp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets/utils/moabb/pipeline.py` around lines 386 - 393, The event time
computation uses raw sample indices directly so it ignores raw.first_samp;
change the math to account for the raw.offset by using (events[:, 0] -
raw.first_samp) / sfreq when computing starts (and similarly for ends entries
derived from events) and compute the final end time using (raw.first_samp +
raw.n_times - 1) / sfreq instead of raw.n_times - 1, so update references to
starts, ends, events, sfreq, raw.first_samp, last_sample and raw.n_times
accordingly.
| hash_obj = hashlib.md5(subject_bytes) | ||
| hash_int = int(hash_obj.hexdigest(), 16) | ||
| bucket = hash_int % n_folds | ||
|
|
||
| assignments = {} | ||
|
|
||
| for k in range(n_folds): | ||
| if bucket == k: | ||
| assignments[f"SubjectSplit_fold{k}"] = "test" | ||
| else: | ||
| fold_str = f"{subject_id}_{seed}_{k}" | ||
| fold_bytes = fold_str.encode("utf-8") | ||
| fold_hash_obj = hashlib.md5(fold_bytes) | ||
| fold_hash_int = int(fold_hash_obj.hexdigest(), 16) |
There was a problem hiding this comment.
Replace hashlib.md5() with hashlib.md5(..., usedforsecurity=False) to avoid FIPS failures.
Both hash calls on lines 541 and 553 omit usedforsecurity=False. On FIPS-compliant systems, MD5 is entirely disabled and these calls raise ValueError at runtime even though MD5 is being used purely for deterministic bucketing, not for any cryptographic purpose.
🔧 Proposed fix
- hash_obj = hashlib.md5(subject_bytes)
+ hash_obj = hashlib.md5(subject_bytes, usedforsecurity=False)
...
- fold_hash_obj = hashlib.md5(fold_bytes)
+ fold_hash_obj = hashlib.md5(fold_bytes, usedforsecurity=False)🧰 Tools
🪛 Ruff (0.15.1)
[error] 541-541: Probable use of insecure hash functions in hashlib: md5
(S324)
[error] 553-553: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@brainsets/utils/split.py` around lines 541 - 554, The MD5 calls using
hashlib.md5(subject_bytes) and hashlib.md5(fold_bytes) should be changed to
include the usedforsecurity flag to avoid FIPS failures: replace
hashlib.md5(subject_bytes) with hashlib.md5(subject_bytes,
usedforsecurity=False) and replace hashlib.md5(fold_bytes) with
hashlib.md5(fold_bytes, usedforsecurity=False); update the variables hash_obj
and fold_hash_obj accordingly so the subsequent hexdigest()/int(...) logic
remains the same.
5 participants
Summary
This PR introduces support for MOABB (Mother of All BCI Benchmarks) datasets, adding two new EEG datasets with a reusable pipeline architecture.
Changes
New Datasets
PhysioNet Motor Imagery (
schalk_wolpaw_physionet_2009)Brain Invaders 2014a P300 (
korczowski_brain_invaders_2014a)New Infrastructure
MOABBPipelinebase class — Reusable pipeline for MOABB datasets handling:New task types in taxonomy:
MOTOR_IMAGERYP300New split utilities:
generate_task_kfold_splits()— Task-specific stratified cross-validationcompute_subject_kfold_assignments()— Deterministic subject-level k-fold assignments (hash-based)Dependencies
New pipeline dependencies:
moabb==1.4.3mne==1.11.0scikit-learn==1.8.0Summary by CodeRabbit
New Features
Tests