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

Computing filters only once (instead per trial) in ft_preprocessing #2219

Open
NirOfir opened this issue Mar 19, 2023 · 1 comment
Open

Computing filters only once (instead per trial) in ft_preprocessing #2219

NirOfir opened this issue Mar 19, 2023 · 1 comment

Comments

@NirOfir
Copy link
Contributor

NirOfir commented Mar 19, 2023

Is your feature request related to a problem? Please describe.
Not a problem, and probably low priority, but I wanted to share in case you dim it worth the effort. I filtered a dataset which was already cut to trials using the 'firws' option, and defined cfg.plotfiltresp = 'yes', because I was still playing with the filter parameters choice. I noticed that FT plots the filter response for each trial, so I looked a bit into the code and noticed that actually the filter is recomputed for each trial, regardless of the filter design method (firws, IIR etc.).

Describe the solution you'd like
Since all trials should be filtered by the same filter, I think it would make more sense to compute the filter once. There are two options that came to my mind:

  1. Compute the filter once in ft_preprocessing() before the loop over trials and pass it to preproc(). Perhaps it would be smart to keep all the filter properties (coefficents, filter type etc.) as a single 'filt' structure within cfg?
  2. Less elegant, but maybe easier: Save the filter properties as persistent variables within preproc() or the specific filtering functions along with variables that will help tell if the user didn't ask for a different filter (something along the lines of the way things work in ft_freqanalysis where wavelets aren't recomputed).

There's potentially quite a lot of work to do to actually implement this change, so I'm not sure I'm up for the task. I wanted to raise this small issue because I saw there's a performance improvement and this seems relevant.

Thanks!

@robertoostenveld
Copy link
Contributor

Thanks for sharing the observation.

Whether to keep the "state" to speed up things is a common challenge. it is for example mentioned here with regard to repeatedly opening files when reading data. The filters for online/realtime application like https://github.com/fieldtrip/fieldtrip/blob/master/preproc/ft_preproc_online_filter_apply.m do keep the state explicitly, but make the user (not a FieldTrip high-level function) responsible for it. In general I like https://peps.python.org/pep-0020/. Keeping state makes it more complex as it distributes the logic over multiple m-files, and reduces readability.

We use persistent variables at more places to maintain state and thereby speed things up, and I think that those offer a decent compromise as they only affect the readability/complexity in a single m-file. Perhaps it would already be a useful starting point to make a FAQ "why do some functions use a persistent variable?", copy some of the considerations here into that FAQ, and search for and add a list of functions. That allows to look at and compare implementations, and to see which code pattern works best; this allows it to be reused more easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants