Skip to content
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

The filter bank paradigm does not correspond to the new abstraction that depends only on the MNE #668

Open
bruAristimunha opened this issue Oct 21, 2024 · 5 comments
Assignees

Comments

@bruAristimunha
Copy link
Collaborator

If you have time after the ICLR review period, it would be great if you could take a look.

@bruAristimunha bruAristimunha changed the title filterbank paradigm is not match the new abstract to relied only on MNE The filter bank paradigm does not correspond to the new abstraction that depends only on the MNE Oct 21, 2024
@PierreGtch
Copy link
Collaborator

@bruAristimunha do you have a bit more details? like a bit of code

First thought that comes to mind: could this be related to edge effects while filtering on epoched data?

@bruAristimunha
Copy link
Collaborator Author

Thanks for replying @PierreGtch, I was thinking about it while looking at the code, and you're right. We have two things here: if we have a paradigm that creates filters correctly, e.g, if we pass the frequency list, we create a bank on the data as a whole.

class FilterBank(BaseMotorImagery):
"""Filter Bank MI."""
def __init__(
self,
filters=([8, 12], [12, 16], [16, 20], [20, 24], [24, 28], [28, 32]),
**kwargs,
):
"""init."""
super().__init__(filters=filters, **kwargs)

And we have a mixer/function that filters the data that has already been epoched, applying the filter at training time, which doesn't necessarily use mne to filter. In my experience, this can lead to errors and difficult checking.

class FilterBank(BaseEstimator, TransformerMixin):

def filterbank(X, sfreq, idx_fb, peaks):

Besides improving documentation, the question now is, what should we do here?

@PierreGtch
Copy link
Collaborator

I think it’s fine to keep both methods, as different users might have different habits. Because of the edge effects, they will not give the same results, so we don’t have much to test here.
The first method (filtering raw signals) gives the most reliable results, so we should present it in the doc and tutorials as the default method for filter banks.
Maybe the second one can simply be listed in the API references, with a disclaimer and a recommendation to use the other one. Also, we could remove this tutorial which uses it:
https://github.com/NeuroTechX/moabb/blob/develop/moabb/pipelines/classification.py

@tomMoral
Copy link
Collaborator

the core question is: does this affect performances of the different pipelines?
In any case, it would be nice to make an example comparing both and explaining the differences.

@PierreGtch
Copy link
Collaborator

This could be a good onboarding project for a Bachelor/Master student :)

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

No branches or pull requests

3 participants