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

[MRG] Add SPDNet #534

Closed
wants to merge 101 commits into from
Closed

[MRG] Add SPDNet #534

wants to merge 101 commits into from

Conversation

tgnassou
Copy link
Contributor

Hi, I started implementing SPDNet in Braindecode, as discussed in the issue #453.
It is just a draft, and the model is not working. I added an example if someone wants to try it. I still need the pooling and batch normalization layers to have the same SPDNet as Huang et al.. I will also add the Riemannian batch norm of Kobler et al. and Brooks et al..

I am new in this area of SPDNet. If anyone has any insights or suggestions, please let me know.

@agramfort
Copy link
Collaborator

Ping @rkobler

@bruAristimunha
Copy link
Collaborator

Heyyy @tgnassou!!

I added the requirements to your example compile. I added Pyriemann, because I planned to push a small tutorial on how to mix braindecode and pyriemann related to my research with @sylvchev. I hope you don't mind.

setup.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #534 (8ca29c1) into master (3247744) will increase coverage by 0.02%.
Report is 20 commits behind head on master.
The diff coverage is 85.71%.

❗ Current head 8ca29c1 differs from pull request most recent head 5e6898b. Consider uploading reports for the commit 5e6898b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   84.72%   84.75%   +0.02%     
==========================================
  Files          63       64       +1     
  Lines        4741     4860     +119     
==========================================
+ Hits         4017     4119     +102     
- Misses        724      741      +17     

@jpaillard
Copy link
Contributor

Hi !
Interesting implementation, thx for sharing.
I was wondering if there is a reason why you adopted Riemannian gradients with geoopt instead of a parameterized optimization approach such as GeoTorch.
According to this paper (theorem 4.3) both approaches theoretically lead to the same outcome but maybe there is a numerical difference ?
If it makes sense I can provide an implementation using GeoTorch for comparison :)

@rkobler
Copy link

rkobler commented Sep 15, 2023

Hi @tgnassou!

Great that you started to implement SPD type models in braindecode. I am happy to support and help in the implementation process!

@jpaillard it would be definitely interesting to test both approaches. So far, I only have experience with geoopt.

tgnassou and others added 14 commits September 18, 2023 15:46
* moving

* Fixing typo and analysing the output

* Fixing

* removing env

* Updating whats new

* dummy commment

* Cleaning
* 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>
@bruAristimunha
Copy link
Collaborator

Things exploded when I updated the branch. I will re-start in a fresh branch/PR @tgnassou

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