Speedups for ft_specest_mtmconvol

[NirOfir]

Is your feature request related to a problem? Please describe.
A few ideas to speed up computation of TFRs and maybe make its feedback more readable. Here are the results of the matlab profiler on data from a single subject with 60 channels sampled at 512 Hz and 795 trials that are 2 seconds long each:

Looking into ft_specest_mtmconvol:

And the specific lines where most time is spent (I broke the convolution into 3 lines to see how long each step takes):

There are a few lines which take the vast majority of the run time:

1. Inverse FFT
2. Multiplication in the frequency domain
3. fftshift()
4. dbstack()

Describe the solution you'd like
Here's what I think can be changed:

1. Adding a 'symmetric' flag to ifft() calls can speed it up, since the input data should always be real. That won't work because the data is real but the wavelets are complex.
2. Since the tapers are calculated once per subject, it seems like it will be much faster to shift the tapers once rather than shift each of the many multiplied spectra.
3. dbstack() is called for every freqeuncy and trial, just to see if the caller function is ft_freqanalysis(), so that ft_progress() can be used. I think we should either pass a relevant flag from ft_freqanalysis() and not call dbstack at all, or just call dbstack once at the start and keep some flag as a persistent variable.
4. 'verbose' is never set by ft_freqanalysis, so it is always on, and the check 'if verbose' (line 337 for example) always passes. Setting 'verbose' to false in ft_freqanalysis if cfg.feedback is no/false makes sense to me, but I don't completely understand how cfg.feedback is treated by fieldtrip.
5. Calculate the feedback string (within the loop over frequencies, to pass to ft_progress) only if 'verbose' is true. This of course depends on fixing 4.
6. 'output time-bins are different from input time-bins' warnings are made for every trial. Since TFRs only have a single time vector for all trials (correct me if I'm wrong), this warning should only be made once, which we can do by using a persistent flag for this warning.

Do my suggestions make sense? I'd be happy to work on a PR.

[schoffelen]

Hi, @NirOfir sorry to be so slow on this, but here are my initial thougths: in general I am a sucker about code efficiency myself, but on the other hand sometimes detailed profiling and code optimization to save a few % of computation time might not be worth the hassle :).

Anyway, I think in general we have to put up with the time spent in ifft'ing the data (which is about half the time that the function needs).

Now, w.r.t. your detailed points:
2. I don't think that this can be done, because the shifting needs to be done after the multiplication (i.e. time domain convolution).
3. I agree, the calls to dbstack (also in the other use case) can be moved to outside all for loops, because the stack does not change within a function call. I'd stay away from persistent variables, and take for granted (for code readability) that the dbstack will be called nrpt number of times, rather than nrpt*nfreq number of times (as is currently the case).
4. I think that a different value of verbose can be achieved if ft_specest_mtmconvol is called directly. W.r.t. cfg.feedback in ft_freqanalysis, the type of feedback is initialized in the caller function (i.e. ft_freqanalysis) and the progress update is done within ft_specest_mtmconvol), I haven't looked into this for a while how this actually works, e.g. if cfg.feedback is specified to 'none'. I tend to agree with your observation that indeed verbose should be passed as an argument into ft_specest_mtmconvol from ft_freqanalysis, because if cfg.feedback is 'none', verbose should be false.
5. ...
6. well, it could be that there are situation where the sampled time axes per trial are different (e.g. due to small shifts introduced by resampling), so it may still be informative to check every trial (althoug probably no one would act upon the warning). Note that ft_warning's verbosity can be tweaked (just like matlab's warning, it has the once option), so you can suppress too many warning by tampering with the notification settings in the global ft_default variable.

I'd be happy to review a PR based on the above :)

[schoffelen]

What's the status of this @NirOfir ?

[NirOfir]

Thanks for the feedback! I'm a bit busy right now, but I hope I can get around to working on a PR in the next month or so. Nothing is too urgent about this issue anyway :)