Skip to content

Filter bank classes #467

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from
Draft

Filter bank classes #467

wants to merge 32 commits into from
+72 −247

Conversation

Sara04
Copy link
Collaborator

@Sara04 Sara04 commented Aug 21, 2023

Hello,

This PR:

  • removes SinglePass class, as it serves only to set-up lower and upper frequencies of the band-pass filter. This can be directly done in the MotorImagery and LeftRightImagery classes.
  • methods of the pair LeftRightImagery & FilterBankLeftRightImagery and the pair MotorImagery & FilterBankMotorImagery are (should be) the same, so there is no need to redefine them. The classes differ only in filters (single vs. filter bank). Therefore, FilterBankLeftRightImagery can inherit LeftRightImagery and FilterBankMotorImagery can inherit MotorImagery. As LeftRightImagery and MotorImagery cannot take as argument 'filters', the attributes of the classes are initialized via grandparent class BaseMotorImagery.
  • is_valid method, should return False if dataset paradigm is not 'imagery'
  • we can use NumpyDocstringInheritanceMeta metaclass to inheret docstrings and in this way avoid long repetitions

Sara Sedlar and others added 22 commits August 1, 2023 19:59
n classes not 2
a8c1918
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
b0a5530
Remove repeating
1a4b71b
Merge branch 'filter_bank_classes' of https://github.com/Sara04/moabb …
b489b90 
…into filter_bank_classes
Update what's new
ec41d89
wrong whats new
6fd8c6d
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
9639253
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
cecb1fc
@Sara04
Remove Band Pass class
da955e8
@Sara04
Merge branch 'filter_bank_classes' of https://github.com/Sara04/moabb …
a60bbb3 
…into filter_bank_classes
@Sara04
Update is valid method
022a891
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
cb07f43
@Sara04
Add docstring inheritance
0d226c8
@Sara04
Merge branch 'filter_bank_classes' of https://github.com/Sara04/moabb …
aaaabec 
…into filter_bank_classes
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
698eb79
@Sara04
Merge branch 'NeuroTechX:develop' into filter_bank_classes
302fcf6
@Sara04
update filterbank MI
3391ea4
@Sara04
Merge branch 'filter_bank_classes' of https://github.com/Sara04/moabb …
314490a 
…into filter_bank_classes
@Sara04
First check, after super init
5f2c768
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
c38fad4
@Sara04
Add n classes argument
54291e1
@Sara04
Merge branch 'filter_bank_classes' of https://github.com/Sara04/moabb …
52eace1 
…into filter_bank_classes
@carraraig carraraig requested a review from PierreGtch October 11, 2023 12:43
PierreGtch
PierreGtch reviewed Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@PierreGtch PierreGtch left a comment

Choose a reason for hiding this comment

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

Hi Sara, I think you should use the class NumpyDocstringInheritanceInitMeta (see comment). If it works, print(MotorImagery.__doc__) should display all the parameters.
And why did you change Parameters to Attributes?
Otherwise, the rest looks good to me!

moabb/paradigms/base.py Outdated
Parameters
Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you change “Parameters” to “Attributes”?

moabb/paradigms/base.py Outdated
"""Base class for paradigms.
Parameters
Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

moabb/paradigms/motor_imagery.py Outdated
"""Motor Imagery for left hand/right hand classification.
Motor imagery paradigm with only one bandpass filter (default 8 to 32 Hz)
Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

moabb/paradigms/motor_imagery.py Outdated
class FilterBankMotorImagery(FilterBank):
"""Filter bank n-class motor imagery.
Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

moabb/paradigms/motor_imagery.py Outdated
"""Filter bank N-class motor imagery.
Parameters
Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Sara04 and others added 3 commits October 19, 2023 16:46
@Sara04 @PierreGtch
Update moabb/utils.py
54487a7 
Change NumpyDocstringInheritanceMeta to NumpyDocstringInheritanceInitMeta class

Co-authored-by: PierreGtch <[email protected]>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
ca6479b
@Sara04 @PierreGtch
Update moabb/utils.py
2c45a05 
Change NumpyDocstringInheritanceMeta to NumpyDocstringInheritanceInitMeta class 2

Co-authored-by: PierreGtch <[email protected]>
@Sara04
Copy link
Collaborator Author

Sara04 commented Oct 19, 2023

Thank you for the review, @PierreGtch ! I've changed 'Parameters' to 'Attributes' as I thought that we use 'Attributes' for class variables and 'Parameters' for methods, but I can revert it to be in accordance with other docstrings.

@bruAristimunha bruAristimunha mentioned this pull request Apr 10, 2024
Open
PierreGtch
PierreGtch reviewed Apr 12, 2024
View reviewed changes
moabb/paradigms/motor_imagery.py
class MotorImagery(SinglePass):
"""N-class motor imagery.
Metric is 'roc-auc' if 2 classes and 'accuracy' if more
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line should stay in the docstring

@PierreGtch
Copy link
Collaborator

PierreGtch commented Apr 12, 2024 •
edited
Loading

@Sara04 Is this PR ready or you wanted to change other things?

I can fix the errors if you want

@Sara04
Copy link
Collaborator Author

Sara04 commented Apr 12, 2024

Hey @PierreGtch , thank you! I will check if there is something more to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PierreGtch PierreGtch PierreGtch left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sara04 @PierreGtch @bruAristimunha