Skip to content

Add support for mne.Epochs in _EEGNeuralNet #529

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 9 commits into from
Sep 14, 2023

Conversation

PierreGtch
Copy link
Collaborator

Passing mne.Epochs will allow to automatically populate more parameters of the model (cf #517 )

@robintibor
Copy link
Contributor

Be aware we are getting rid of mne epochs for efficiency reasons in #515

@PierreGtch
Copy link
Collaborator Author

PierreGtch commented Sep 11, 2023 via email

@robintibor
Copy link
Contributor

Hm no idea was to move away from mne epochs entirely within the braindecode dataset classes, both for code simplicity as well as due to long times instantiating mne epochs in the create windows functions. But here it looks like anyways there are no braindecode dataset classes involved? might be good to have both some examples of this for docs as well as tests so it can be better understood :)

@PierreGtch
Copy link
Collaborator Author

Exactly, here there is no braindecode dataset involved. It would just allow to do this:

epo = mne.Epochs(...)
y = ...
net = EEGClassifier(...)
net.fit(epochs, y)

It's only to facilitate the usage for small datasets.

@bruAristimunha
Copy link
Collaborator

I loved =)

@bruAristimunha
Copy link
Collaborator

Elaborating more... I think it's an interesting counterproposal to what Robin is removing... We removed this internal support to make it more external, this way the neuro user can use it more easily. Do you agree @robintibor?

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #529 (db09571) into master (4acecaf) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head db09571 differs from pull request most recent head 56f7779. Consider uploading reports for the commit 56f7779 to get more accurate results

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage   84.51%   84.51%   -0.01%     
==========================================
  Files          63       63              
  Lines        4664     4662       -2     
==========================================
- Hits         3942     3940       -2     
  Misses        722      722              

@robintibor
Copy link
Contributor

In general yes, I think right now for me hard to keep overview over everything, once we have things implemented with examples we should see from user perspective whether things are clear. Like also if it's clear when to use which option etc. Also from developer perspective how much overhead we have from multiple ways of specifying the data... Here this part seems minimal so that seems good :)

bruAristimunha added a commit that referenced this pull request Sep 12, 2023
* Set signal-related parameters in check_data

* Merge tests for EEGClassifier and EEGRegressor

* Fix coquille

* Update test to use module mixin

* Rename clf to eegneuralnet in test

* Add preds fixture

* Update eegneuralnet.py and subclasses

* Update test_eegneuralnet.py

* restored previous tests behavior where nn layer is ignored and always returns mocked values

* Update whats_new.rst

* Use two different mock modules for test

* Rename mock modules

* Use set_params instead of vars

* Deprecate passing an initialized module and skip setting signal args in that case

* Remove unnecessary f-strings

* Try fix python 3.8

* Fix case with non torch dataset

* Try fix python 3.8

* Add docstrings for fit and partial_fit (already including #529)

* Test initialized module

* Fix Flake8

* Add email

* Remove deprecation of initialized module

---------

Co-authored-by: Maciej Sliwowski <macieksliwowski@gmail.com>
Co-authored-by: Bru <a.bruno@aluno.ufabc.edu.br>
@PierreGtch PierreGtch marked this pull request as ready for review September 12, 2023 22:09
@bruAristimunha bruAristimunha merged commit a4c896d into braindecode:master Sep 14, 2023
@bruAristimunha
Copy link
Collaborator

Thank you @PierreGtch and @robintibor =)

tgnassou pushed a commit to tgnassou/braindecode that referenced this pull request Sep 18, 2023
* Set signal-related parameters in check_data

* Merge tests for EEGClassifier and EEGRegressor

* Fix coquille

* Update test to use module mixin

* Rename clf to eegneuralnet in test

* Add preds fixture

* Update eegneuralnet.py and subclasses

* Update test_eegneuralnet.py

* restored previous tests behavior where nn layer is ignored and always returns mocked values

* Update whats_new.rst

* Use two different mock modules for test

* Rename mock modules

* Use set_params instead of vars

* Deprecate passing an initialized module and skip setting signal args in that case

* Remove unnecessary f-strings

* Try fix python 3.8

* Fix case with non torch dataset

* Try fix python 3.8

* Add docstrings for fit and partial_fit (already including braindecode#529)

* Test initialized module

* Fix Flake8

* Add email

* Remove deprecation of initialized module

---------

Co-authored-by: Maciej Sliwowski <macieksliwowski@gmail.com>
Co-authored-by: Bru <a.bruno@aluno.ufabc.edu.br>
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.

3 participants