adding EEG-ITNET model

[GhBlg]

adding the model of EEG-ITNet

[agramfort]

@GhBlg can you add tests as we do for other models? 🙏 @GhBlg

[codecov]

Codecov Report

Merging #400 (0573834) into master (a5e52d5) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   82.63%   82.81%   +0.18%     
==========================================
  Files          53       54       +1     
  Lines        3738     3789      +51     
==========================================
+ Hits         3089     3138      +49     
- Misses        649      651       +2     

[bruAristimunha]

Hello @GhBlg,

Nice addition to the library! I made some changes to make the review easier, let's wait for someone more senior on the team to review and request some adjustments!

Thank you very much for your work!

[GhBlg]

My pleasure!
Thank you very much for your help!

[sliwy]

@GhBlg many thanks for this contribution. We always dream about having all the SOTA models in braindecode! 😄

For the implementation, I would reorganize the code a little bit and use classes to make the implementation shorter and clearer. By using two classes (_InceptionBlock and _TCBlock) we can have less redundant code. It would also correspond better to the structure introduced in the paper. Now it can be a little bit tricky to spot the difference between different blocks.

Let me know, what you think about my propositions and if you have any questions! I am happy to help! 😄

[GhBlg]

Thank you very much for your help and your review, the code is way better now 😊 !

[sliwy]

@GhBlg great job! Thanks for that! I think we may have a second iteration (after my review) to make it perfect, if you don't mind! 😄

Two things for now:

1. It would be easier for me to review if you can take a look at checks that happen in this PR thread - especially flake8 for now. If you now look at the changes in this PR you can see some errors from flake8. It would be awesome if you can fix that.

2. You can also resolve the comments that you have solved so it is easier for you to know what is still missing and for us to review.

If you have any questions, don't hesitate to respond under my comments! :)

[sliwy]

@GhBlg I think that a few small reorganization may improve the code.

Main points (everything is in the comments as well):

1. Let's put residual connection logic in the _TCBlock
2. A little bit changed _InceptionBlock that could be more generic.
3. Use of add_module method from nn.Sequential to add new modules.

Could you resolve the conversation when fixed? 🙏 Would be much easier to discuss then. Thanks a lot for contributing once again!

[GhBlg]

Thank you for your reply ! I will try to fix the current issues.

[bruAristimunha]

Hey @GhBlg,

Hope you don't mind, but I added one more test.

[GhBlg]

yeah sure ! thank you for adding it !

[sliwy]

@GhBlg I pushed to your branch with some minor formatting/parameter name change. :)

I also added drop_prob param in the TC_block definition part to match the original implementation.

I am happy with the changes, thanks for the effort @GhBlg!

[GhBlg]

I am very glad for helping to contribute in this great library ! Thank you very much @agramfort @sliwy @bruAristimunha 🙏

[bruAristimunha]

@agramfort, can you apply the merge when the tests are done?

[bruAristimunha]

Looks like codecov/project has crashed or enter in loop.

[agramfort]

@GhBlg can you just add an entry with your contribution to the what’s new page ? So people know what you did for the project 🙏

[agramfort]

In the page https://braindecode.org/whats_new.html @GhBlg

[GhBlg]

done 😊

[agramfort]

thx @GhBlg

[bruAristimunha]

Thank you very much for the new model implemented, @GhBlg

Welcome to the library contributors list =) 🚀

Always feel invite to collaborate and suggest changes. Your participation is important for the growth of the library. Please let us know if you have ideas for more contributions to the library and need any help.

Many thanks for the reviews @sliwy and @agramfort 😄.

[GhBlg]

Thank you very much for helping me merging the model @agramfort @bruAristimunha @sliwy ❤️ ! I hope to continue adding more state-of-the-art contributions in the near future ! 😄

[martinwimpff]

@GhBlg Thank you for adding this model to the library!
One small thing that I discovered during a debugging session: the original implementation uses a weight normalization in the DepthwiseConv layers of the inception branches. This detail was not mentioned in the paper, so your implementation is perfectly in line with the paper.

I ran some experiments with the BCIC IV 2a dataset to check the importance of this additional weight norm. In my experiments using weight norm did not really have an impact on the overall accuracy (when using one value for all subjects) but if one would tune this parameter per subject there would be some improvements.

So should we add weight norm to this model?

[GhBlg]

@martinwimpff Thank you very much for the tests !
Actually, I think it can be a good idea to add it with an optional argument to activate the weight normalization.