EpochsTFR objects computed with the multitaper method & complex output have a taper dimension which needs to be aggregated across, however the weights for these tapers are not stored in the EpochsTFR object and need to be re-computed within spectral_connectivity_time().

For these weights to be accurate, we need to know the bandwidth and number of cycles used to compute the original TFR. However, these are also not stored in the EpochsTFR object, so the user would need to pass this information in to the mt_bandwidth and n_cycles params.

I have noted this in the docstring, but I thought it would also be useful that if an EpochsTFR object with multitaper method is passed in and the default parameters are used (mt_bandwidth=None and n_cycles=7.0), then a warning is raised. Only annoying thing is that the n_cycles warnign is unavoidable if 7 cycles were actually used to compute the TFR.

Maybe we should add the mt_bandwidth and n_cycles attributes to the *TFR objects?

It would be nice to have. I'm happy to open a PR if people think it's justified.

Provenance is usually nice to track and if it's easy enough to add to the class then we might as well. Might be worth a quick MNE issue as we might want them as attributes like tfr.n_cycles but maybe better would be something like a dict of params like tfr.estimation_parameters = {'mt_bandwidth': 4, 'n_cycles': 5} so that the tfr namespace itself doesn't get too busy.

Sure, will open an issue first then.

maybe a naive question, but if what you need is the taper weights, why not store those directly rather than storing the other estimation params necessary to recompute them? It's not a perfect solution because TFRs computed from earlier versions of MNE and then saved to disk may still lack the necessary information... but the same is true of storing the estimation parameters as attributes (instead of the weights).

Copying from the discussion in mne-tools/mne-python#12851 (comment) for reference:

drammock: Since we already store weights for spectrum objects, I'm inclined to store weights (not estimation params) for TFRs too. That saves having to recompute them when users pass MNE objects (not arrays) to the connectivity functions, and is more internally consistent.

tsbinns: Sure, I see your reasoning, especially to keep it consistent with *Spectrum objects. I'll open a PR for this.

statements like this worry me, because they mean that it's easy for people to make mistakes.

For things that are ignored if you pass an EpochsTFR object, then (assuming the default param value is None or some other null/auto value) we at least have the option of checking whether non-default input to this function matches what we extract/infer from the object, and raising warnings/errors if they don't match. But for params that have int/float default values, or for quantities/settings that were used to create the TFR object but aren't stored with it, we can't even do that sanity check.

I don't want to throw a big monkey wrench into this PR, but an API like this makes me think that there should be separate functions for operating on MNE Objects and on arrays, and the shared code path should be a helper func rather than the user-facing function(s). Another possibility is 2 public-facing funcs and no shared helper; one of the public funcs would then call the other after suitable transformation of its inputs. This could mean internally extracting arrays from objects and always operating on arrays, or it could mean internally promoting arrays to objects and always operating on objects.

Without looking closely I'm not sure how much work that would be / how (in)consistent it is with the rest of the mne_connectivity API. I also apologize if this was discussed in advance and I missed/forgot about that discussion... I've been rather underwater for the last several months with other responsibilities. But I'd like to at least check with you and @larsoner (and maybe @adam2392 if he has time) before this becomes something that needs a deprecation cycle to change.

(side note re: the discussion elsewhere in this PR between @tsbinns and @larsoner about storing estimation params like n_cycles as part of the TFR object --- that helps prevent some mistakes, but I still have the feeling like this function is trying to do too much.)

statements like this worry me, because they mean that it's easy for people to make mistakes.

I 100% agree. With storing the params (or the weights directly like discussed in mne-tools/mne-python#12851), then this statement could be removed and prevent possible mistakes.

but I still have the feeling like this function is trying to do too much.

But yeah, I also understand.

an API like this makes me think that there should be separate functions for operating on MNE Objects and on arrays

Just to clarify, this would mean something like:

• spec_conn_epochs/time handling Epochs and EpochsSpectrum/TFR objects
• and a spec_conn_epochs/time_array function handling array of timeseries data or coefficients

If there were separate functions, would it not be simpler to maintain spec_conn_epochs/time as handling timeseries data (either as Epochs or arrays), and introduce equivalent functions for handling spectral coefficients (either as EpochsSpectrum, EpochsTFR, or arrays)? I feel like this would be more compatible with the suggestion below:

Another possibility is 2 public-facing funcs and no shared helper; one of the public funcs would then call the other after suitable transformation of its inputs. This could mean internally extracting arrays from objects and always operating on arrays, or it could mean internally promoting arrays to objects and always operating on objects.

However for the second sentence, I think again it's less a case of distinguishing data in arrays vs. MNE objects, and more distinguishing timeseries vs. coefficients.

Without looking closely I'm not sure how much work that would be / how (in)consistent it is with the rest of the mne_connectivity API.

I imagine the most important thing is for whatever is changed in spec_conn_time, that should be mirrored in spec_conn_epochs and vice versa (so things like #220 would need to be looked at again). I am perfectly happy to work on this (but I also understand that means more work for reviewers).

I think again it's less a case of distinguishing data in arrays vs. MNE objects, and more distinguishing timeseries vs. coefficients.

yeah you're right I was confusing the issue. Just to make sure we're on the same page: am I right that the "flavors" of spectral connectivity you want to support are:

• input: non-spectral (ordinary time series)
  • across: epochs (spec_conn_epochs; objects or arrays)
  • across: times (spec_conn_time; objects or arrays)
• input: spectral
  • across: epochs (currently spec_conn_epochs; maybe a new func?)
    domain)
  • across: time (not supported; no time dimension)
• input: spectrotemporal
  • across: epochs (not supported?)
  • across: times (this PR: spec_conn_time; maybe a new func? spec_conn_tfr_time?)

input: non-spectral (ordinary time series)

• across: epochs (spec_conn_epochs; objects or arrays)
• across: times (spec_conn_time; objects or arrays)

Yes.

input: spectral

• across: epochs (currently spec_conn_epochs; maybe a new func?)
  domain)
• across: time (not supported; no time dimension)

Yes.

input: spectrotemporal

• across: epochs (not supported?)
• across: times (this PR: spec_conn_time; maybe a new func? spec_conn_tfr_time?)

This PR was also adding support for spectrotemporal input for spec_conn_epochs to match the cwt_morlet mode.

maybe a naive question, but if what you need is the taper weights, why not store those directly rather than storing the other estimation params necessary to recompute them? It's not a perfect solution because TFRs computed from earlier versions of MNE and then saved to disk may still lack the necessary information... but the same is true of storing the estimation parameters as attributes (instead of the weights).