[MRG] Add optional different ways of averaging EpochsTFR by adam2392 · Pull Request #8879 · mne-tools/mne-python

adam2392 · 2021-02-16T21:12:36Z

Reference issue

Closes: #8878

What does this implement/fix?

Essentially same workflow as Epochs.average().

mne/time_frequency/tfr.py

adam2392 · 2021-02-19T17:52:12Z

Not sure why CI failed. Locally,

(mne) adam2392@Adams-MacBook-Pro mne-python % pytest ./mne/time_frequency/tests 
====================================================== test session starts =======================================================
platform darwin -- Python 3.8.5, pytest-6.2.1, py-1.9.0, pluggy-0.13.1
rootdir: /Users/adam2392/Documents/mne-python, configfile: setup.cfg
plugins: xdist-2.1.0, cov-2.10.1, profiling-1.7.0, timeout-1.4.2, forked-1.3.0
collected 87 items                                                                                                               

mne/time_frequency/tests/test_ar.py s.                                                                                     [  2%]
mne/time_frequency/tests/test_csd.py ..............                                                                        [ 18%]
mne/time_frequency/tests/test_multitaper.py ss                                                                             [ 20%]
mne/time_frequency/tests/test_psd.py ......                                                                                [ 27%]
mne/time_frequency/tests/test_stft.py ........................................                                             [ 73%]
mne/time_frequency/tests/test_stockwell.py .....                                                                           [ 79%]
mne/time_frequency/tests/test_tfr.py ..................                                                                    [100%]

Module-level timings require pytest-harvest


--------------------------- generated xml file: /Users/adam2392/Documents/mne-python/junit-results.xml ---------------------------
====================================================== slowest 20 durations ======================================================
4.02s call     mne/time_frequency/tests/test_tfr.py::test_getitem_epochsTFR
2.61s call     mne/time_frequency/tests/test_tfr.py::test_plot_joint
1.15s call     mne/time_frequency/tests/test_tfr.py::test_plot
0.81s call     mne/time_frequency/tests/test_stockwell.py::test_stockwell_ctf
0.61s call     mne/time_frequency/tests/test_psd.py::test_psd
0.61s call     mne/time_frequency/tests/test_tfr.py::test_io
0.61s call     mne/time_frequency/tests/test_tfr.py::test_compute_tfr
0.46s call     mne/time_frequency/tests/test_tfr.py::test_tfr_ctf
0.40s call     mne/time_frequency/tests/test_tfr.py::test_time_frequency
0.39s call     mne/time_frequency/tests/test_stockwell.py::test_stockwell_api
0.20s call     mne/time_frequency/tests/test_tfr.py::test_tfr_multitaper
0.20s call     mne/time_frequency/tests/test_psd.py::test_psd_welch_average_kwarg[raw]
0.18s call     mne/time_frequency/tests/test_psd.py::test_psd_welch_average_kwarg[epochs]
0.18s call     mne/time_frequency/tests/test_psd.py::test_psd_welch_average_kwarg[evoked]
0.18s call     mne/time_frequency/tests/test_csd.py::test_csd_multitaper
0.16s call     mne/time_frequency/tests/test_ar.py::test_ar_raw
0.13s call     mne/time_frequency/tests/test_csd.py::test_csd_fourier
0.04s call     mne/time_frequency/tests/test_psd.py::test_compares_psd
0.04s call     mne/time_frequency/tests/test_stockwell.py::test_stockwell_core
0.03s call     mne/time_frequency/tests/test_csd.py::test_csd_morlet
==================================================== short test summary info =====================================================
SKIPPED [1] mne/time_frequency/tests/test_ar.py:17: Requires statsmodels version >= 0.8
SKIPPED [1] mne/time_frequency/tests/test_multitaper.py:14: Test test_dpss_windows skipped, requires nitime. Got exception (No module named 'nitime')
SKIPPED [1] mne/time_frequency/tests/test_multitaper.py:39: Test test_multitaper_psd skipped, requires nitime. Got exception (No module named 'nitime')
================================================= 84 passed, 3 skipped in 15.60s =================================================

larsoner · 2021-02-19T18:04:53Z

It's pip pre which uses prerelease versions of libraries. It looks unrelated. @drammock can you look? It has to do with latest matplotlib breaking something about our drawing / clicking / interaction, not sure what. Probably some upstream change modified how things get drawn or something.

agramfort · 2021-02-21T13:54:01Z

@adam2392 can you add/update a test?

larsoner · 2021-02-22T13:24:57Z

We have to be a little bit careful here because of the output option -- median on output='complex' data will almost certainly not work properly as is, see numpy/numpy#12943 / scipy/scipy#12669 / scipy/scipy#12676. I think we can fix by:

In the docstring add a warning to Notes about not passing np.median as it's unsafe.
If 'median' string-option is used, internally we need to separate into real and imaginary parts, take medians, then recombine. We can do this by defining a _median_complex in mne.fixes (I think eventually a fix will land in NumPy along these lines) that triages based on the iscomplexobj like in the SciPy PR, and wrap to that call rather than directly to np.median.
Add to the notes that we use the marginal median in the complex case

agramfort · 2021-02-25T09:25:41Z

mne/time_frequency/tfr.py

+        -----
+        Passing in ``median`` is considered unsafe when there is complex
+        data. Use with caution. We use the marginal median in the
+        complex case.


can you clarify this? should we even accept it then? What is marginal median, can you better explain
or provide a ref?

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:

scipy/scipy#12676 (comment)

Updated docstring in 9ad5741

agramfort · 2021-02-25T12:55:40Z

let's document this ! thx @larsoner

…

agramfort

are the remaining CI issues expected?

agramfort · 2021-02-28T10:56:00Z

mne/time_frequency/tfr.py

+
+            >>> 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


have you tested trim_mean with complex numbers? it behaves as expected?

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"

mne/fixes.py

mne/time_frequency/tfr.py

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

agramfort · 2021-03-01T08:22:48Z

@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)

agramfort reviewed Feb 19, 2021

View reviewed changes

mne/time_frequency/tfr.py Show resolved Hide resolved

agramfort reviewed Feb 19, 2021

View reviewed changes

mne/time_frequency/tfr.py Outdated Show resolved Hide resolved

adam2392 added 5 commits February 19, 2021 11:10

Adding time frequency pr.

fb4a58a

Adding change entry.

a37b707

Clean up docstring.

6815655

Fixing docstring.

8c37d9c

Fix docstring.

99602ce

adam2392 force-pushed the epochstfr branch from 6dff0d6 to 99602ce Compare February 19, 2021 16:11

adam2392 requested a review from agramfort February 19, 2021 17:39

adam2392 added 2 commits February 21, 2021 18:25

Adding unit tests.

1584135

Adding docstring.

605745a

adam2392 added 2 commits February 22, 2021 11:54

Fixing docstring.

51af4d6

Fixi flake.

c68a745

agramfort reviewed Feb 25, 2021

View reviewed changes

adam2392 added 3 commits February 25, 2021 10:23

Merging main.

3dae672

Fix docstring.

cdc54ae

Fix docstring.

9ad5741

agramfort reviewed Feb 28, 2021

View reviewed changes

adam2392 and others added 3 commits February 28, 2021 11:55

Merge branch 'main' into epochstfr

93a6fa1

Update mne/fixes.py

c9a3d90

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

Fix docstring.

b981556

agramfort approved these changes Mar 1, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into epochstfr

bbafc83

* 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)

agramfort merged commit 2b09a17 into mne-tools:main Mar 2, 2021

adam2392 deleted the epochstfr branch August 25, 2021 01:31

attack68 mentioned this pull request Oct 2, 2021

BUG: Groupby.median returns absurd results for complex pandas-dev/pandas#43770

Open

3 tasks

Uh oh!

Conversation

adam2392 commented Feb 16, 2021

Reference issue

What does this implement/fix?

Uh oh!

Uh oh!

Uh oh!

adam2392 commented Feb 19, 2021

Uh oh!

larsoner commented Feb 19, 2021

Uh oh!

agramfort commented Feb 21, 2021

Uh oh!

larsoner commented Feb 22, 2021

Uh oh!

agramfort Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

larsoner Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

adam2392 Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

agramfort commented Feb 25, 2021 via email

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

adam2392 Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

agramfort commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants