-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[MRG] Add optional different ways of averaging EpochsTFR #8879
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.
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
Conversation
|
Not sure why CI failed. Locally, |
|
It's |
|
@adam2392 can you add/update a test? |
|
We have to be a little bit careful here because of the
|
mne/time_frequency/tfr.py
Outdated
| ----- | ||
| Passing in ``median`` is considered unsafe when there is complex | ||
| data. Use with caution. We use the marginal median in the | ||
| complex case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify this? should we even accept it then? What is marginal median, can you better explain
or provide a ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing in the string 'median' should be fine, internally we should notice this and compute the marginal median. Passing in np.median is unsafe (and should be documented as unsafe here) because NumPy don't compute the marginal median, they sort the complex values by real part and return whatever value is computed.
The marginal median is the median of each component separately, which is easy to compute:
https://en.wikipedia.org/wiki/Median#Marginal_median
This is the way we are going for SciPy:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docstring in 9ad5741
agramfort
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the remaining CI issues expected?
mne/time_frequency/tfr.py
Outdated
|
|
||
| >>> from scipy.stats import trim_mean # doctest:+SKIP | ||
| >>> trim = lambda x: trim_mean(x, 0.1, axis=0) # doctest:+SKIP | ||
| >>> epochs_tfr.average(method=trim) # doctest:+SKIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested trim_mean with complex numbers? it behaves as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good point. I'll take that out for now since it was just a copy/paste of Epochs, which is only real data. The main plus here in this PR was to allow one to use np.median, for being "robust to outliers"
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
|
@larsoner feel free to merge if CI failures are expected. I did not keep track of which ones to ignore. thx |
* upstream/main: MAINT: Skip matplotlib pre for now (mne-tools#8973) FIX: Brain lights (mne-tools#8972) MNT: Migrate VTK Widgets (mne-tools#8862) Fix (mne-tools#8971)
* upstream/main: MAINT: Revert conda azure change (mne-tools#8978) make large rotation penalty optional in chpi function (mne-tools#8770) Fix: Wrong channel-adjacency-matrix for Neuromag122 (mne-tools#8891) [MRG] Add optional different ways of averaging EpochsTFR (mne-tools#8879) DOC, STY: fix dataframe scrolling (mne-tools#8977)
Reference issue
Closes: #8878
What does this implement/fix?
Essentially same workflow as
Epochs.average().