Skip to content
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

Not using mne epochs for windowing #515

Merged
merged 32 commits into from Sep 27, 2023

Conversation

robintibor
Copy link
Contributor

Not calling mne.Epochs construction anymore during windowing,using directly the computed crop indices on the mne raw.
For now, in same WindowsDataset class as before, potentially splitting the class into two later.

Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! A few comments.

braindecode/datasets/base.py Outdated Show resolved Hide resolved
braindecode/datasets/base.py Outdated Show resolved Hide resolved
braindecode/datasets/base.py Show resolved Hide resolved
braindecode/datasets/base.py Outdated Show resolved Hide resolved
braindecode/datasets/base.py Show resolved Hide resolved
braindecode/preprocessing/windowers.py Outdated Show resolved Hide resolved
braindecode/preprocessing/windowers.py Show resolved Hide resolved
braindecode/preprocessing/windowers.py Outdated Show resolved Hide resolved
test/unit_tests/preprocessing/test_windowers.py Outdated Show resolved Hide resolved
@robintibor
Copy link
Contributor Author

Still a lot of code duplication and code simplification to do, will tackle that next

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #515 (4058072) into master (cb5cad0) will decrease coverage by 0.86%.
Report is 3 commits behind head on master.
The diff coverage is 97.34%.

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
- Coverage   84.55%   83.70%   -0.86%     
==========================================
  Files          63       63              
  Lines        4676     4812     +136     
==========================================
+ Hits         3954     4028      +74     
- Misses        722      784      +62     

@tomMoral
Copy link
Collaborator

yes, brackets on new lines makes more happy 😁

@bruAristimunha
Copy link
Collaborator

Hey @robintibor!

From the CI it looks like it is finished, is there something missing in the PR?

@robintibor
Copy link
Contributor Author

So still need to reenable the outcommented test or remove it, and possibly rename dataset classes, but might do renaming of dataset classes in separate PR

@robintibor
Copy link
Contributor Author

Trying to get at it today

@robintibor
Copy link
Contributor Author

So this would be ready from my side now I think @bruAristimunha feel free to merge. See also new issues I created #543 and #544

@bruAristimunha bruAristimunha merged commit bea45b0 into braindecode:master Sep 27, 2023
16 of 17 checks passed
@bruAristimunha
Copy link
Collaborator

Amazing @robintibor 🚀!!! Thank you so much for the code and review, @robintibor, @tomMoral and @sliwy =)

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.

None yet

4 participants