Skip to content

Not using mne epochs for windowing #515

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

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.

@robintibor robintibor force-pushed the windower-without-epochs branch from db0a4d1 to f9480e8 Compare September 8, 2023 13:03
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.

@robintibor robintibor force-pushed the windower-without-epochs branch from 0bf207c to 51fbb44 Compare September 11, 2023 15:02
@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
@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.

4 participants