-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
ENH: return complex multitaper output per taper #10281
Conversation
FYI the plan in the next year or two is to overhaul these functions so that there are proper containers to hold different variants of spectral information, probably including complex tapers https://mne.tools/stable/overview/roadmap.html#time-frequency-classes IIRC this will include both pure frequency (spectral) and time-resolved (time-frequency) estimates. @drammock is going to work on this mostly, but he's out for a bit. That being said, given how long it might take to get all the above work done, this seems like a good intermediate solution. I agree we should not allow |
98d4860
to
c08302c
Compare
@larsoner do you think it would be useful to provide per taper results for edit: yes, I think so - currently asking for |
…ves the same as output='power'
Either way is fine by me. It can be added here, or assume YAGNI and we can add it later if someone wants it |
@larsoner ready for review. Previous tests were green but the build failed on docs timeout. |
mne/time_frequency/tests/test_tfr.py
Outdated
print(multitaper_complex.shape) | ||
print(multitaper_power.shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cruft? But probably okay since pytest
will capture it anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, yes :) I will remove it.
mne/time_frequency/tfr.py
Outdated
@@ -380,6 +385,8 @@ def _compute_tfr(epoch_data, freqs, sfreq=1.0, method='morlet', | |||
|
|||
if ('avg_' in output) or ('itc' in output): | |||
out = np.empty((n_chans, n_freqs, n_times), dtype) | |||
elif output in ['complex', 'phase'] and n_tapers > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it makes more sense to always output the tapers
dim, even if it's a singleton. It makes code and .ndim
work the same regardless of if the time-bandwidth product / half-bandwidth (or whatever we call it) changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree, I'll change that
cb4e3e6
to
9b1a189
Compare
@larsoner All done and green. :) |
Thanks @mmagnuski ! |
Thanks @larsoner 🚀 |
Reference issue
Fixes #8722.
What does this implement/fix?
Changes multitaper time-frequency to return per-taper Fourier coefficients when
output == 'complex'
. So far the coefficients were averaged across tapers which is not correct. This change should also allow users to calculate connectivity measures per taper and average across tapers (this is what we do correctly now for ITC).Additional information
Currently the output has the following shape:
(n_epochs, n_channels, n_tapers, n_freqs, n_times)
This could be changed if you think a different output structure will work better.
To see how this PR works, you can try the script below (based on code provided by @chapochn in #8722; I will reshape this code into a test later this week):
TODOs:
output='phase'
disablecomplex or phase results seem to not be available throughoutput='complex'
for normal (non-array) tfr multitaper?tfr_multitaper