-
Notifications
You must be signed in to change notification settings - Fork 8
Update P3 tutorial #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Update P3 tutorial #192
Conversation
VivianChencwy
commented
Nov 6, 2025
- Migrated to EEGDash for data import
- Code cleanup
- Complete tutorial documentation
- Completed and verified the HTML documentation build
bruAristimunha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More one iteration @VivianChencwy, FYI @arnodelorme
| class ManualWindowsDataset: | ||
| """Custom dataset that ensures one window per event.""" | ||
|
|
||
| def __init__(self, data, labels): | ||
| self.data = data | ||
| self.labels = labels | ||
|
|
||
| def __len__(self): | ||
| return len(self.data) | ||
|
|
||
| def __getitem__(self, idx): | ||
| return self.data[idx], self.labels[idx] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| class ManualWindowsDataset: | |
| """Custom dataset that ensures one window per event.""" | |
| def __init__(self, data, labels): | |
| self.data = data | |
| self.labels = labels | |
| def __len__(self): | |
| return len(self.data) | |
| def __getitem__(self, idx): | |
| return self.data[idx], self.labels[idx] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is much better, but we need to iterate:
I'll continue with some general comments for now, and later I'll help you finish/help you more by committing directly:
-
Let's remove the ManualWindowsDataset object; we'll use the baseconcat dataset.
-
Similarly, let's use the existing functions in braindecode pre-processing to do the pre-processing, removing the OddballPreprocessor, which seems to only rename events and apply some processing. You can check in the startkit how we did this renaming and adapt your logic. In theory, it will be a function that you apply to the pre-processing.
-
Let's also remove the logic of processing by subject; here, we will apply this to each raw data.
-
For the model, let's import the eegconformer from braindecode, okay? Unless there's a reason why we need to redefine it.
Let's address these points first, and then we'll move on to training.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed feedback!
I’ve implemented the requested changes:
- Removed ManualWindowsDataset — now using braindecode's BaseConcatDataset directly via create_windows_from_events()
- Removed OddballPreprocessor — replaced with a simple rename_oddball_events() function used in the braindecode preprocessing pipeline via create_preprocessors()
- Removed per-subject processing logic — load_combined_arrays() now applies preprocessing directly to each raw recording using preprocess(dataset, preprocessors)
- Imported EEGConformer from braindecode — removed custom model definitions and now using from braindecode.models import EEGConformer
The code is ready for review. If there are any other modifications needed, please let me know.