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.

Sign up for GitHub

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

Jump to bottom

[BUG] Fix taper weighting in computation of TFR multitaper power #13067

Merged
merged 4 commits into from
Jan 19, 2025
Merged

[BUG] Fix taper weighting in computation of TFR multitaper power #13067

merged 4 commits into from
Jan 19, 2025

Conversation

tsbinns
Copy link
Contributor

@tsbinns tsbinns commented Jan 16, 2025

Reference issue (if any)

Fixes #13023

What does this implement/fix?

When computing TFRs using the multitaper method, taper weights were not being applied to the tapered spectra when converting complex coefficients in to power.
This PR makes sure the taper weights are applied, along with unit tests.

@tsbinns
Copy link
Contributor Author

tsbinns commented Jan 17, 2025

Looks like some tests just timed out and others failed for unrelated reasons.

drammock
drammock approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

LGTM, just one idea that you're free to follow or ignore.

@drammock
Copy link
Member

pip-pre error (both linux and windows) looks unrelated:

FAILED mne/preprocessing/tests/test_ica.py::test_read_ica_eeglab_mismatch - scipy.io.matlab._miobase.MatWriteWarning: Starting field name with a underscore (__header__) is ignored

@larsoner larsoner added the backport-candidate on-merge: backport to maint/1.9 label Jan 17, 2025
@larsoner larsoner enabled auto-merge (squash) January 17, 2025 21:44
@larsoner larsoner merged commit 8b9fc97 into mne-tools:main Jan 19, 2025
32 checks passed
@lumberbot-app Lumberbot (App)
Copy link

lumberbot-app bot commented Jan 19, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout maint/1.9
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 8b9fc973e0bdaca9a5ba0c9333637722ed323633
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13067: [BUG] Fix taper weighting in computation of TFR multitaper power'
  1. Push to a named branch:
git push YOURFORK maint/1.9:auto-backport-of-pr-13067-on-maint/1.9
  1. Create a PR against branch maint/1.9, I would have named this PR:

"Backport PR #13067 on branch maint/1.9 ([BUG] Fix taper weighting in computation of TFR multitaper power)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

qian-chu pushed a commit to qian-chu/mne-python that referenced this pull request Jan 20, 2025
@tsbinns @larsoner @qian-chu
[BUG] Fix taper weighting in computation of TFR multitaper power (mne…
388a426 
…-tools#13067)

Co-authored-by: Eric Larson <[email protected]>
tsbinns added a commit to tsbinns/mne-python that referenced this pull request Jan 20, 2025
@tsbinns @larsoner
Backport PR mne-tools#13067 on branch maint/1.9 ([BUG] Fix taper weig…
485b939 
…hting in computation of TFR multitaper power)

Co-Authored-By: Eric Larson <[email protected]>
tsbinns added a commit to tsbinns/mne-python that referenced this pull request Jan 20, 2025
@tsbinns @larsoner
Backport PR mne-tools#13067 on branch maint/1.9 ([BUG] Fix taper weig…
d4f57b2 
…hting in computation of TFR multitaper power)

Co-Authored-By: Eric Larson <[email protected]>
@tsbinns tsbinns mentioned this pull request Jan 20, 2025
Merged
@tsbinns
Copy link
Contributor Author

tsbinns commented Jan 20, 2025

Have had a go at resolving the merge conflicts and manually backporting in #13072.

larsoner added a commit that referenced this pull request Jan 21, 2025
@tsbinns @larsoner
Backport PR #13067 on branch maint/1.9 ([BUG] Fix taper weighting in …
96d22f8 
…computation of TFR multitaper power) (#13072)

Co-authored-by: Eric Larson <[email protected]>
@tsbinns tsbinns deleted the fix_tfr_mt_weighting branch January 21, 2025 23:09
larsoner added a commit to larsoner/mne-python that referenced this pull request Jan 24, 2025
@larsoner
Merge remote-tracking branch 'upstream/main' into cola5
46c193d 
* upstream/main: (57 commits)
  Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071)
  MAINT: Fix doc build (mne-tools#13076)
  BUG: Improve sklearn compliance (mne-tools#13065)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13073)
  MAINT: Add Numba to 3.13 test (mne-tools#13075)
  Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071)
  [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656)
  New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037)
  [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067)
  [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069)
  Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13060)
  BUG: Fix bug with interval calculation (mne-tools#13062)
  [DOC] extend documentation for add_channels (mne-tools#13051)
  Add `combine_tfr` to API (mne-tools#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058)
  BUG: Fix bug with helium anon (mne-tools#13056)
  [ENH] Add option to store and return TFR taper weights (mne-tools#12910)
  BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828)
  MAINT: Ensure limited set of tests are skipped (mne-tools#13053)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock approved these changes

@larsoner larsoner larsoner approved these changes

@adam2392 adam2392 Awaiting requested review from adam2392 adam2392 is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

Assignees
No one assigned
Labels
backport-candidate on-merge: backport to maint/1.9 Still Needs Manual Backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible bug in computation of multitaper TFR power
3 participants
@tsbinns @drammock @larsoner