[GSOC] Add `EpochsTFR` support to spectral connectivity functions

[tsbinns]

WIP follow up of #225 to finalise GSoC work.

So far adds support for coefficients from Epochs.compute_tfr(method="morlet", output="complex") to spectral_connectivity_epochs(), equivalent to the cwt_morlet mode. Still to be done is adding support for EpochsTFR objects in spectral_connectivity_time().

In addition to the Morlet approach, spectral_connectivity_time() supports the time-freq. multitaper mode. #126 could also be addressed by adding support for this to spectral_connectivity_epochs(), in the form of EpochsTFR objects.

However, when trying this I discovered a bug that prevents the time-freq. multitaper mode being computed from epoched data (https://github.com/mne-tools/mne-python/issues/12831), but that seems like an easy fix.

Will continue to work on this next week.

[tsbinns]

Have now added support for EpochsTFR to spectral_connectivity_time (using a custom MNE branch with the bug in #12831 fixed).

[tsbinns]

Resetting tests to main mne branch now that https://github.com/mne-tools/mne-python/pull/12842 is merged

[tsbinns]

Thanks for the detailed review @drammock! Until those wider API questions are addressed I'll double check the unit tests and try to find where the deviation in Fourier/Welch pipelines.

[tsbinns]

Conda test keeps freezing on the micromamba setup step, seems similar to this: https://github.com/mamba-org/setup-micromamba/issues/225 Is it worth trying to fix now?

[larsoner]

Yeah feel free! Sounds like there might be a workaround via pinning in some of those issue comments

[tsbinns]

Haven't forgotten about this, just need to sort https://github.com/mne-tools/mne-python/pull/12910 before I can finalise the changes and tests here.

[tsbinns]

https://github.com/mne-tools/mne-python/pull/12910 is now merged, so I can fix the outstanding issues here.

[tsbinns]

I've addressed the previous issues.

There should now be full support for EpochsSpectrum and EpochsTFR data in spectral_connectivity_epochs (including time-resolved multitaper coeffs) and for EpochsTFR in spectral_connectivity_time, along with unit tests covering the changes.

[tsbinns]

Given that the multitaper TFR requires the unreleased MNE 1.10, is it worth holding off on merging (once it's checked and ready to go) until after MNE-Conn 0.8 is released?

On that note, since the last major part of the GSoC project in #223 was merged, there's a decent amount for a new release: https://mne.tools/mne-connectivity/dev/whats_new.html What do people think?

Opened an issue here -> #276

[larsoner]

Given that the multitaper TFR requires the unreleased MNE 1.10, is it worth holding off on merging (once it's checked and ready to go) until after MNE-Conn 0.8 is released?

You could, but even then you'll need to add a version guard to the mne-connectivity that checks the MNE version. You have them in tests but you also need them in the user-facing function. You can use something like _soft_import("mne", "multitaper connectivity", min_version="1.10") for it

[larsoner]

... although I think the min_version in _soft_import was added recently, so better just to use

if not check_version("mne", "1.10):
    raise ...

in the user-facing code

[tsbinns]

You could, but even then you'll need to add a version guard to the mne-connectivity that checks the MNE version. You have them in tests but you also need them in the user-facing function. You can use something like _soft_import("mne", "multitaper connectivity", min_version="1.10") for it

Currently I'm checking compatibility based on the object attrs. Do you think an explicit version check is cleaner?

https://github.com/mne-tools/mne-connectivity/blob/73ce0a6bc4ea164656d36af049119090fbd07e06/mne_connectivity/spectral/epochs.py#L1225-L1233

[drammock]

Currently I'm checking compatibility based on the object attrs. Do you think an explicit version check is cleaner?

Is the issue just about which MNE version is currently installed? Or is it to do with which version of MNE the data were saved to disk with? If the latter plays a role, then attr-based checking seems right to me (though I'd tweak the error message to explicitly reference the possibility of loaded data that were saved by an older version of MNE). On the other hand if it's just about the currently installed version of MNE, then checking that (rather than object attrs) makes for easier-to-maintain code IMO (as the code more directly reflects the source of the problem).

[tsbinns]

Is the issue just about which MNE version is currently installed? Or is it to do with which version of MNE the data were saved to disk with?

Moreso the latter, whatever MNE version was used to compute the coefficients. But thinking about it, could be the case that a multitaper EpochsSpectrum/TFR object saved from an older MNE version and loaded with a newer version has a weights attr that is None, so that check would fail. A more thorough check would be:

if not hasattr(data, "weights") or (data.weights is None and mode == "multitaper"):
    # XXX: Remove logic when support for mne<1.10 is dropped 
    raise AttributeError( 
       "weights are required for multitaper coefficients stored in " 
       "EpochsSpectrum (requires mne >= 1.8) and EpochsTFR (requires " 
       "mne >= 1.10) objects" 
    ) 

I'd tweak the error message to explicitly reference the possibility of loaded data that were saved by an older version of MNE

Yeah good call, I'll add this.

[drammock]

@larsoner this one LGTM, I'll let you merge if happy

[larsoner]

Thanks @tsbinns !

[tsbinns]

Thanks @drammock and @larsoner for the patience with this!