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

add fast bandpass filter #29

Merged
merged 2 commits into from
Feb 9, 2022
Merged

Conversation

timonmerk
Copy link
Contributor

@timonmerk timonmerk commented Mar 19, 2021

Based on #26 I want to push a faster bandpass filter estimation than the mne.filter.filter_data.

I also wrote a notebook where I compare the two on my private repo:
https://github.com/neuromodulation/py_neuromodulation/blob/main/examples/speedtests/speedtest_filter_vs_MNE.ipynb

There you can see that the initialized rt_filter.py estimation took ~6 ms while the mne.filter.filter_data took ~31 ms. Both is based on a sampling frequency of 500 Hz and 6 channels.

@teonbrooks Do you think it might be a good idea to upload such an example as well maybe in a folder called "real time tests"?

Copy link
Member

@teonbrooks teonbrooks left a comment

Choose a reason for hiding this comment

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

this first pass is mostly to address some styling for the code.

mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
filter_bank : array
filter coefficients stored in array of shape (n_franges, filter_len (in samples))
"""
filter_list = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
filter_list = []
filter_list = list()

mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
mne_realtime/rt_filter.py Show resolved Hide resolved
mne_realtime/rt_filter.py Show resolved Hide resolved
mne_realtime/rt_filter.py Show resolved Hide resolved
mne_realtime/rt_filter.py Outdated Show resolved Hide resolved
@larsoner
Copy link
Member

There you can see that the initialized rt_filter.py estimation took ~6 ms while the mne.filter.filter_data took ~31 ms. Both is based on a sampling frequency of 500 Hz and 6 channels.

I would much rather speed up mne.filter.filter_data for this case rather than add a new function. It's cleaner conceptually and also will benefit more people (anyone using filter_data on similar data).

In this case I'm guessing that a direct call to scipy.signal.lfilter to do the filtering would be faster than mne.filter.filter_data, but it's worth figuring out the conditions under which each method is fastest based on FIR filter length and using lfilter or our internal overlap-add method when appropriate.

@timonmerk
Copy link
Contributor Author

timonmerk commented Mar 19, 2021

@teonbrooks Thanks a lot for the code review! I will adapt it.
@larsoner That's a good point, in my implementation it get's only faster since I am precalculating in calc_band_filters() the filters using mne.filter.create_filter.

Then I only "apply" them in a np convolution in def apply_filter(). Probably this is not an optimal implementation for mne-python, but speeds up computation for real time analysis :)

@larsoner
Copy link
Member

That's a good point, in my implementation it get's only faster since I am precalculating in calc_band_filters() the filters using mne.filter.create_filter.

Ahh, and we don't provide a way to pass the FIR filter directly. We do this with IIR but not FIR...

We could in theory provide some way to do this in MNE. For maximum efficiency you could even compute the FFT of the filter once, and use it with the signal. We'd have to figure out an API for it, though.

@timonmerk
Copy link
Contributor Author

@larsoner Do you mean the iir_params keyword in mne.filter.filter_data? Also I am not sure about the FFT approach. This would also require a frequency transformed version of the data itself?

Sorry if I don't really get this. For my intuition the fast approach would be simply to do the filtering in the time domain via a convolution with the precalculated filterbank.

Base automatically changed from master to main March 19, 2021 21:49
@timonmerk
Copy link
Contributor Author

Alright, so I adapted some changes, simply because of the nice comments of @teonbrooks :)
I am not sure though if this should be included in mne real-time now because of the upper discussion. If so I think a module in test might make sense?
Similar to my private speed test here https://github.com/neuromodulation/py_neuromodulation/blob/main/examples/speedtests/speedtest_filter_vs_MNE.ipynb

@larsoner
Copy link
Member

I am not sure though if this should be included in mne real-time now because of the upper discussion.

I think for now just add a comment along the lines that "someday MNE-Python could add support for precomputed FIR filters like it does for IIR" and in the meantime a PR just using np.convolve directly is fine. You might also want to note that fftconvolve or overlap-add (MNE-Python uses the latter) will be faster in some cases so if this ends up being a bottleneck there are options for optimization.

Then this PR can just focus on getting the API right and getting it to work, and optimization / refactoring of MNE-Python can be done later if someone gets motivated to do it by performance concerns.

@teonbrooks
Copy link
Member

hey @timonmerk, I will have next week to work on the project. sorry i've run away for a while. If you want to make some final adjustment, I can then merge these and we can iterate on a faster cadence.

@teonbrooks teonbrooks merged commit c7b959f into mne-tools:main Feb 9, 2022
@teonbrooks
Copy link
Member

sorry for the delay @timonmerk. thanks!

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.

3 participants