-
Notifications
You must be signed in to change notification settings - Fork 173
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
[WIP] EEG-Inception #390
[WIP] EEG-Inception #390
Conversation
Codecov Report
@@ Coverage Diff @@
## master #390 +/- ##
==========================================
+ Coverage 82.81% 83.03% +0.21%
==========================================
Files 54 55 +1
Lines 3789 3838 +49
==========================================
+ Hits 3138 3187 +49
Misses 651 651 |
In general more models are very suitable thank you very much!! Can you add some tests with different input sizes just checking forward works and output shape is as expected? |
great stuff as @robintibor would say :) to get this merged we would need a test (see how done for others) and a what's new entry btw I checked the license of https://github.com/Grifcc/EEG/tree/90e412a407c5242dfc953d5ffb490bdb32faf022 and it's Apache2 so it's ok for here (BSD). |
Sure, I will do the tests. Glad I'm helping =) |
Do you need any support here? :) Is this still ongoing? :) |
thanks for reminding me @robintibor, I will try to finish this week. |
Hallo @robintibor! I think I'm going to need some help X= |
Hello @sliwy, how are you? I'm also in need of code review. I'm doing something wrong in the code and it's not working for dimensions other than the default value. |
Hi @bruAristimunha , I may try to help. I need more info from you when it fails, what is the error, maybe how to reproduce. I think that we may be able to simplify a little bit by creating blocks and using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruAristimunha really great contribution with the Inception model, needed in the library for sure!
braindecode/models/eeginception.py
Outdated
from .functions import squeeze_final_output | ||
|
||
|
||
class CustomPad(nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that, we can use it as well in this PR #400
braindecode/models/eeginception.py
Outdated
self.sfreq = sfreq | ||
self.n_filters = n_filters | ||
|
||
scales_samples = [int(s * sfreq / input_window_samples) for s in scales_time] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this should depend on input_window_samples
. I think that if we increase the size of the input window we may still want to keep the same scales, corresponding to length of selected miliseconds.
Maybe we can define kernel lengths in the inception in seconds. To me it is more natural.
Hi @sliwy, Strangely, the change to sequential increases the number of parameters. It still has a weird error when changing input values. I re-added the tests that were failing. I have no idea what's wrong or how to fix. I think I'll wait for the conclusion of the other PR and adapt based on the other code. The networks are similar. |
@bruAristimunha to debug easily the number of parameters change in the layers you can use https://github.com/TylerYep/torchinfo |
I refactored it based on the other model, and here is the result of the torchinfo summary. I will go into more depth on this issue, but possibly it will take me a while.
|
thank you so much! I hadn't noticed the wrong formatting, I think I'll stop for today ;p in fact, I think I sent a duplicate. But good idea, I'll do it (module vs sequential) |
Hello guys, @robintibor and @sliwy, I've talked to @cedricrommel, and he's going to take a closer look at this code for the next week. |
I found a few problems with the layer dimensions and fix them. Both tests are now passing. For the second test I found in the authors official code provided by @bruAristimunha that they set all biases to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruAristimunha can you add this model to the doc before merging?
🙏
Apparently |
Done @agramfort and @cedricrommel, Many thanks, @sliwy and @robintibor, for the review, and thank you, @cedricrommel, for the hard work on this model! I spent a lot of time trying to find these little details, and I am pleased that you, as an expert professional, managed to find them. Happy to contribute to the library with this model that won last year's competition. I am open for further revisions if you feel it necessary, @agramfort, @robintibor and @sliwy. |
I am proposing to implement EEG-Inception. This model was used by the winners of last year BEETL Competition.
Is this suitable for library?