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

[ENH, MRG] Add compute_csd function #10986

Merged
merged 19 commits into from
Aug 16, 2022
Merged

[ENH, MRG] Add compute_csd function #10986

merged 19 commits into from
Aug 16, 2022

Conversation

alexrockhill
Copy link
Contributor

@alexrockhill alexrockhill commented Aug 1, 2022

Part of #10920.

This uses power to compute the cross-spectral density. It would be nice if this could be done on complex numbers but that's causing issues with positive semi-definite matrices. It might be nice to just go ahead with this version and then improve it later since it matches how other functions like csd_morlet do CSD and then modify all the functions to be consistent in a followup PR.

@alexrockhill
Copy link
Contributor Author

Failed tests are because this depends on #10979

@alexrockhill
Copy link
Contributor Author

@wmvanvliet if you have a minute to review this one too that would be really great :)

@alexrockhill alexrockhill reopened this Aug 3, 2022
@alexrockhill alexrockhill changed the title [ENH, MRG] Add compute_csd function [ENH, WIP] Add compute_csd function Aug 3, 2022
@alexrockhill
Copy link
Contributor Author

Needs to check that it gives the same results as csd_morlet etc., I think there's a scaling issue.

@alexrockhill
Copy link
Contributor Author

alexrockhill commented Aug 3, 2022

Okay, so this passes tests locally that calling tfr_morlet(epochs) and then compute_csd(epochs_tfr) is the same as csd_morlet(epochs) by changing it to match. But, a couple important questions remain:

  • Does this code work for csd_multitaper (csd_fourier I think is not applicable)? I wasn't able to check because the parameterization is currently different (tfr_multitaper takes n_cycles whereas csd_multitaper takes an fmin and fmax), @drammock do you know if there's a backward compatible way to match the fmin/fmax parameterization with the freqs/n_cycles parameterization?
  • Would this theoretically work with Hilbert transformed data? Maybe this is an empirical question to compare with the filtered raw data approach, tagging @britta-wstnr as I think she might be interested
  • There are many different methods in mne.compute_covariance, can we allow those as well? It takes epochs with dimension (n_epochs, n_channels, n_times) so it would need to take 4D input or somehow handle frequencies. The way that I had it in the previous commits returned something sensible but didn't match csd_morlet even when normalized so I think there's some scaling going on that may make things different but it would be interesting to know if those give better results.

I think question 1 should be addressed before merging but questions 2 and 3 are more long-term trying to figure out how best to do this kind of thing.

@alexrockhill
Copy link
Contributor Author

I took a bit more of a look at it appears that tfr_mutlitaper doesn't even accept an output argument to get complex data. This was easy to add (you have to average across the tapers which I'm not sure works really well for phase though) but did not give quantitatively similar results using the same frequencies (you still have the freqs + n_cycles parameterization vs the fmin/fmax parameterization) so I think it does not work. Up to people who maintain csd functions more whether to go ahead and merge knowing that it only works for Morlet or add a warning that it only works for EpochsTFR derived from tfr_morlet or dig into the multitaper more or what. This is a nice efficiency improvement for applying DICS to time-frequency epochs though so it would be nice to get merged at some point.

@alexrockhill alexrockhill changed the title [ENH, WIP] Add compute_csd function [ENH, MRG] Add compute_csd function Aug 10, 2022
@alexrockhill
Copy link
Contributor Author

Okay to merge? This follows the definition of CSD here

image

(From @britta-wstnr et al. 's 2022 Neuroimage paper and pointed out by her)

mne/time_frequency/csd.py Outdated Show resolved Hide resolved
mne/time_frequency/csd.py Outdated Show resolved Hide resolved
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

doc/time_frequency.rst Outdated Show resolved Hide resolved
mne/time_frequency/csd.py Outdated Show resolved Hide resolved
mne/time_frequency/tests/test_csd.py Outdated Show resolved Hide resolved
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Just marking as changes requested until my comments above are addressed. CI failure is unrelated and should be fixed by #11034 eventually

@alexrockhill
Copy link
Contributor Author

Thanks for the review @larsoner, I added those changes. Now, https://github.com/mne-tools/mne-python/blob/main/mne/time_frequency/csd.py#L1344-L1345 is done the old none-einsum way (thanks for figuring that out by the way, I was pretty close actually but just didn't get the right combination of the first index being the only different one, I had the first and second and both the same but now that you wrote the correct one, it seems so easy) but it's done per channel so it's a bit more refactoring than should probably go in this PR.

@alexrockhill
Copy link
Contributor Author

Looks good to go unless I missed anything

@larsoner larsoner merged commit 530988c into mne-tools:main Aug 16, 2022
@larsoner
Copy link
Member

Thanks @alexrockhill !

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.

None yet

3 participants