Skip to content

[MNT] Eegnetv4 fix v2 - #680

Merged
sylvchev merged 27 commits into
braindecode:masterfrom
bruAristimunha:eegnetv4-fix-v2
Feb 26, 2025
Merged

sylvchev merged 27 commits into
braindecode:masterfrom
bruAristimunha:eegnetv4-fix-v2

Conversation

@bruAristimunha

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Oct 22, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (1c1b845) to head (f5caf9b).
Report is 8 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #680   +/-   ##
=======================================
  Coverage   87.25%   87.25%           
=======================================
  Files          76       76           
  Lines        6833     6833           
=======================================
  Hits         5962     5962           
  Misses        871      871           

@bruAristimunha bruAristimunha changed the title [MNT} Eegnetv4 fix v2 [MNT] Eegnetv4 fix v2 Oct 28, 2024
@bruAristimunha

Copy link
Copy Markdown
Collaborator Author

I talked again with Victor from the speechbrain team. We checked the model diff again again based on the torchinfo summary, and the only remaining difference is the convolution order that needs to be re-checked.

@bruAristimunha

Copy link
Copy Markdown
Collaborator Author

The benchmark table @tomMoral
image
image
image

@bruAristimunha

bruAristimunha commented Feb 5, 2025 •

Copy link
Copy Markdown
Collaborator Author

Hey @tomMoral,

I did a more in-depth study for EEGNet because the benchmark I initially did was slightly different from the original result. (different evaluation)

In the original article, we have interfered from the barplot that the accuracy is 68% +-[70%-67%] (Within-Subject, mixing two sessions and doing cross-validation with four folds (conceptually wrong for me)) and 38% +- [42%-35%] (inter-subjects).

Using benchopt, I investigated the impact of different band frequencies (one of the key differences following our table):
intra_session
inter_session
inter_subject

Using only the closest frequency range [4Hz, 38Hz], we have better results.

objective_name use_linear_layer=False use_linear_layer=True original
inter_sessions 44.5+-19.96 59.38+-15.63
inter_subjects 51.15+-6.31 49.31+-8.51 38% +- [42%-35%]
intra_session 47.09+-23.21 62.87+-20.77
within subject - 68% +-[70%-67%]

There are several interesting things about this contrast of different evaluations, more or less filtering and domain generalization, and possibly, because of this, the implementation has not been corrected before. Since linear is better when we filter less, I will keep it in my ideas box to explore in the future, somewhat along the lines of our article in the CNS.

We should integrate this code that adds this parameter, and during the benchmark, we validate whether we leave it as linear as default or not.

@PierreGtch PierreGtch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice job @bruAristimunha !! I'm just not sure about the parameter name and the default. WDYT?

Comment thread braindecode/models/eegnet.py
Comment thread braindecode/models/eegnet.py Outdated
Comment thread braindecode/models/modules.py
Comment thread braindecode/models/eegnet.py Outdated
Comment thread braindecode/models/eegnet.py Outdated
Comment thread braindecode/models/eegnet.py Outdated
Comment thread braindecode/models/eegnet.py Outdated
Comment thread braindecode/models/eegnet.py Outdated
bruAristimunha and others added 3 commits February 25, 2025 08:59
Co-authored-by: Pierre Guetschel <25532709+PierreGtch@users.noreply.github.com>
@bruAristimunha

Copy link
Copy Markdown
Collaborator Author

Hi @PierreGtch and @tomMoral,

I appreciate your review @PierreGtch. I have incorporated everything. I really like the new name, thank you for that. As for changing the name, I would rather not. Let's wait for a release to do that.

@sylvchev

Copy link
Copy Markdown
Collaborator

Thanks @bruAristimunha !

@sylvchev
sylvchev merged commit c7a07b4 into braindecode:master Feb 26, 2025
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