Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix PSD weights handling when bad annotations present #12538

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

drammock
Copy link
Member

follow-up to #12536

  • corrects bug where weights were discarded for spans shorter than n_per_seg (the wrapped function actually just shrinks n_per_seg in those cases)
  • reduces weights for long spans to the number of used samples (where we expect trailing samples to be discarded)

@cbrnr
Copy link
Contributor

cbrnr commented Apr 10, 2024

I can confirm that this fixes the error from #11413. I do get a warning though:

UserWarning: nperseg = 2048 is greater than input length  = 1201, using nperseg = 1201

I understand that this is expected and technically correct, but from a user perspective we are just trying to compute a spectrum (using data with annotations), so I'm not sure if this warning causes more confusion than anything else. Would an info message not be sufficient here?

@drammock
Copy link
Member Author

I'm not sure if this warning causes more confusion than anything else. Would an info message not be sufficient here?

message comes from scipy, not from MNE. We could catch and swallow the warning maybe? Or replace it with an info log message that is a bit more informative.

@larsoner
Copy link
Member

We could catch and swallow the warning maybe? Or replace it with an info log message that is a bit more informative.

I'm fine with swallowing the warning or re-emitting. Instead of re-emitting it's probably even cleaner to just detect the too-short condition in code (since you have direct access to all the lengths) and emit a message that way rather than parse the obtained warnings or whatever.

@cbrnr
Copy link
Contributor

cbrnr commented Apr 11, 2024

Somehow I still see the UserWarning:

>>> raw.plot_psd()
NOTE: plot_psd() is a legacy function. New code should use .compute_psd().plot().
Setting 1201 of 6007 (19.99%) samples to NaN, retaining 4806 (80.01%) samples.
Effective window size : 3.410 (s)
At least one good data span is shorter than n_per_seg, and will be analyzed with a shorter window than the rest of the file.
/Users/clemens/Projects/mne-python/mne/time_frequency/psd.py:52: UserWarning: nperseg = 2048 is greater than input length  = 1201, using nperseg = 1201
  _, _, spect = func(epoch)
<stdin>:1: FutureWarning: The value of `amplitude='auto'` will be removed in MNE 1.8.0, and the new default will be `amplitude=False`.
Plotting power spectral density (dB=True).
<MNELineFigure size 2000x700 with 2 Axes>

For reference, here's the MWE again:

import mne
from mne.datasets import sample

fname = sample.data_path() / "MEG" / "sample" / "sample_audvis_raw.fif"
raw = mne.io.read_raw(fname).pick_types(eeg=True).crop(0, 10).load_data()

raw.plot_psd()  # works
raw.set_annotations(mne.Annotations(2, 2, "bad"))
raw.plot_psd()  # UserWarning

@drammock
Copy link
Member Author

Argh, I didn't see that when I tried. I'll look again. The warnings catcher comes with a caveat that its behavior is undefined in multi threaded contexts, so I may need to handle the warning at a lower level (maybe wrap the scipy func directly)

@drammock
Copy link
Member Author

OK, CIs are green and the warning-catcher now immediately wraps scipy.signal.spectrogram so it should be immune to multi-threading issues.

@larsoner larsoner merged commit bf74c04 into mne-tools:main Apr 11, 2024
30 checks passed
@larsoner
Copy link
Member

Thanks @drammock !

@drammock drammock deleted the followup branch April 12, 2024 14:28
larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 15, 2024
* upstream/main: (50 commits)
  ENH: Improve OPM auditory dataset and example (mne-tools#12539)
  MAINT: Bump to latest pydata-sphinx-theme (mne-tools#12228)
  MRG: Simplify manual installation instructions a little by dropping explicit mention of (lib)mamba (mne-tools#12362)
  fix PSD weights handling when bad annotations present (mne-tools#12538)
  Fix phase loading (mne-tools#12537)
  align FFT windows to good data spans in psd_array_welch (mne-tools#12536)
  explicitly disallow multitaper in presence of bad annotations (mne-tools#12535)
  MAINT: Clean up PyVista contexts (mne-tools#12533)
  MAINT: Complete API change of ordered (mne-tools#12534)
  MAINT: Reinstall statsmodels and improve logging (mne-tools#12532)
  MAINT: Remove scipy.signal.morlet2 (mne-tools#12531)
  Update README badge links (mne-tools#12529)
  BUG: Fix bug with reading his_id from snirf (mne-tools#12526)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12524)
  Fix file format check in _check_eeglab_fname function (mne-tools#12523)
  MAINT: Reenable picard in pre testing (mne-tools#12525)
  MAINT: Bump to large resource class (mne-tools#12522)
  MAINT: Restore 2 jobs on Windows (mne-tools#12520)
  Add exclude_after_unique option to mne.io.read_raw_edf/read_raw_edf to search for exclude channels after making channel names unique (mne-tools#12518)
  Improve consistency of sensor types in code and documentation (mne-tools#12509)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants