Skip to content

[BUG] Allow Epochs.compute_tfr() for the multitaper method and complex/phase outputs#12842

Merged
larsoner merged 6 commits intomne-tools:mainfrom
tsbinns:fix_tfr_multitaper
Sep 16, 2024
Merged

[BUG] Allow Epochs.compute_tfr() for the multitaper method and complex/phase outputs#12842
larsoner merged 6 commits intomne-tools:mainfrom
tsbinns:fix_tfr_multitaper

Conversation

@tsbinns
Copy link
Copy Markdown
Contributor

@tsbinns tsbinns commented Sep 10, 2024

Reference issue

Fixes #12831.

What does this implement/fix?

Fixes a bug where an internal check of the shape of TFR results was unexpectedly failing for results with both an epochs and tapers (multitaper method and complex/phase output) dimension.

Additional information

The check for whether an epochs dimension is present cannot rely on self._dims, as "epoch" only gets added afterwards, so it is instead based on the data passed in (i.e., is it an epochs object?). We don't have to worry about whether an epochs dimension is still present (e.g., if compute_tfr() was called with average=True), since average=True is not allowed when output="complex"/"phase".

There is currently no test checking the combination of multitaper method and complex/phase output, and I haven't added one here to avoid overburdening the test suite. If this is required I can add one.

@larsoner
Copy link
Copy Markdown
Member

There is currently no test checking the combination of multitaper method and complex/phase output, and I haven't added one here to avoid overburdening the test suite. If this is required I can add one.

Yes please do add it -- if the number of time points and channels is low it should be plenty fast, and it would be good to catch corner cases like this one!

@tsbinns
Copy link
Copy Markdown
Contributor Author

tsbinns commented Sep 16, 2024

Just added a very short test that multitaper complex/phase TFR runs.

@larsoner
Copy link
Copy Markdown
Member

Marking for merge-when-green, thanks in advance @tsbinns !

@larsoner larsoner enabled auto-merge (squash) September 16, 2024 10:10
@tsbinns
Copy link
Copy Markdown
Contributor Author

tsbinns commented Sep 16, 2024

Seems some sklearn compliance checks are still failing.

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.

Epochs.compute_tfr() fails for complex and phase outputs in the multitaper method

2 participants