[BUG] Cross-spectral density missing frequencies#12633
Merged
larsoner merged 2 commits intomne-tools:mainfrom May 29, 2024
Merged
[BUG] Cross-spectral density missing frequencies#12633larsoner merged 2 commits intomne-tools:mainfrom
larsoner merged 2 commits intomne-tools:mainfrom
Conversation
mscheltienne
approved these changes
May 29, 2024
Member
mscheltienne
left a comment
There was a problem hiding this comment.
Great explanation in this PR, thank you!
Member
|
Just tacking on a little test here... @meeseeksdev Hello |
|
Look at me, @larsoner, I'm Mr. Meeseeks! |
Member
|
@meeseeksdev backport to maint/1.7 |
meeseeksmachine
pushed a commit
to meeseeksmachine/mne-python
that referenced
this pull request
May 29, 2024
larsoner
pushed a commit
that referenced
this pull request
May 29, 2024
…missing frequencies) (#12634) Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference issue
Discussed in call with @larsoner & @wmvanvliet.
What does this implement/fix?
Currently, computing the CSD with the
csd_multitaper,csd_fourier,csd_array_multitaper, orcsd_array_fourierfunctions of thetime_frequencymodule will return aCrossSpectralDensityobject whose frequencies do not match thefminandfmaxparameters supplied to the functions.E.g. calling the functions with
fmin=5, fmax=10could be expected to return a CSD object with frequencies[5, 6, 7, 8, 9, 10](assuming a 1 Hz resolution). Instead, the following frequencies are returned[6, 7, 8, 9].This occurs because of the logic used to generate the internal frequency mask:
As such, the
fminandfmaxentries of 5 and 10 Hz may be included in the list of possible frequencies, but they are ignored when generating the frequency mask (e.g. as 5 is not greater than itself, and 10 is not less than itself).A simple solution if to allow entries of
orig_frequenciesto also matchfminandfmax, e.g.:mne-python/mne/time_frequency/csd.py
Lines 814 to 816 in 74e7348
This change ensures that
fmin=5, fmax=10will return a CSD object with frequencies[5, 6, 7, 8, 9, 10]. Note that(orig_frequencies > 0)is important to prevent the CSD being returned for the zero frequency, which would not match the behaviour of other spectral methods (e.g. PSD computation).@larsoner This does not include logic for cases that can be considered as floating point errors, but if I recall you said this was optional.
This requires a small change to the unit tests for catching failing cases where no frequency bins are captured due to
fminandfmaxbeing too close, e.g.:https://github.com/tsbinns/mne-python/blob/74e7348be7536b42bca2fa3ce48ecd4f0536a89b/mne/time_frequency/tests/test_csd.py#L397