Skip to content

Implementation of ATCnet - #429

Merged
cedricrommel merged 17 commits into
braindecode:masterfrom
cedricrommel:ATCNet
Nov 25, 2022
Merged

Implementation of ATCnet#429
cedricrommel merged 17 commits into
braindecode:masterfrom
cedricrommel:ATCNet

Conversation

@cedricrommel

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Nov 15, 2022

Copy link
Copy Markdown

Codecov Report

Merging #429 (6002542) into master (171e505) will increase coverage by 0.59%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   84.81%   85.40%   +0.59%     
==========================================
  Files          48       49       +1     
  Lines        3728     3913     +185     
==========================================
+ Hits         3162     3342     +180     
- Misses        566      571       +5     

@cedricrommel

Copy link
Copy Markdown
Collaborator Author

ping @tomMoral @agramfort :)

@agramfort agramfort 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.

LGTM although I would make all classes in ATCNet.py with a starting _ to be private except the ATCNet class that is public.

🙏 @cedricrommel

Comment thread braindecode/models/atcnet.py Outdated
Comment thread braindecode/models/atcnet.py Outdated
Co-authored-by: robintibor <robintibor@gmail.com>
@cedricrommel

cedricrommel commented Nov 25, 2022

Copy link
Copy Markdown
Collaborator Author

LGTM although I would make all classes in ATCNet.py with a starting _ to be private except the ATCNet class that is public.

🙏 @cedricrommel

Thanks Alex! I didn't add the _ in some modules because I thought those were general enough to be useful in other future networks. I reckon I should have moved them into braindecode.models.modules :) I'll do it for those that still seem general enough and add the _ to the others.

@cedricrommel

Copy link
Copy Markdown
Collaborator Author

Feel free to merge if it looks good to you :)

@tomMoral tomMoral 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.

LGTM.

Just one nitpick but this is on me, I don't like the warpping style when subclassing does the job. feel free to ignore.

Comment thread braindecode/models/modules.py Outdated
@tomMoral

Copy link
Copy Markdown
Collaborator

Same can be done with the maxnorm layer.

Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
@cedricrommel

Copy link
Copy Markdown
Collaborator Author

Same can be done with the maxnorm layer.

Good point! Did the same for MaxNormLiner

@cedricrommel
cedricrommel merged commit 114c3ec into braindecode:master Nov 25, 2022
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.

4 participants