[MRG] Fix compare evokeds #4755

agramfort · 2017-11-15T05:14:09Z

@jona-sassenhagen @ktavabi I took over both of your PRs and simplify to refactor / simplify

please have a look

jona-sassenhagen · 2017-11-15T05:15:30Z

mne/viz/evoked.py

-                         ylim=dict(), invert_y=False, show_sensors=None,
-                         show_legend=True, axes=None, title=None, show=True):
+def _combine_grad(evoked, picks):
+    """Creates a new instance of Evoked with combined gradiometers (RMSE)"""


Uuuuh, that's good to have.

jona-sassenhagen · 2017-11-15T05:16:21Z

mne/viz/evoked.py

+
+
+def _check_loc_legal(loc, what='your choice'):
+    """Check if a loc is a legal for MPL."""


"""Check if loc is a legal location for MPL subordinate axes."""
(or something like that, it's missing a noun)

jona-sassenhagen · 2017-11-15T05:17:24Z

mne/viz/evoked.py

+
+
+def _format_evokeds_colors(evokeds, cmap, colors):
+    # set up labels and instances to have evokeds as a dict


Sorry, what? :) I don't get what this sentence means.

jona-sassenhagen · 2017-11-15T05:18:08Z

mne/viz/evoked.py

+    channels using global field power (GFP) computation, else it is taking
+    a plain mean.
+
+    This function is useful for comparing ER[P/F]s at a specific location.


This function is useful for comparing multiple ER[P/F]s - e.g., for multiple conditions - at a specific location.

jona-sassenhagen · 2017-11-15T05:20:30Z

mne/viz/evoked.py

+    It can plot a simple Evoked object or if supplied with a list or a dict
+    of lists of evoked instances, it will compute the average of the mean or
+    GFP across datasets. It can also compute some confidence intervals
+    using for example bootstrap estimates.


It plots 1. a simple evoked object, 2. a list or dict of evoked objects (e.g., for multiple conditions), 3. a list or dict of lists of evokeds (e.g., for multiple subjects in multiple conditions). In the last case, it can show a confidence interval (across e.g. subjects) using parametric or bootstrap estimation.

jona-sassenhagen · 2017-11-15T05:21:58Z

mne/viz/evoked.py

-    show_legend : bool | int
-        If not False, show a legend. If int, the position of the axes
+    show_legend : bool | str | int
+        If not False, show a legend. If int or str, the position of the axes


the position of the legend
or
the position of the legend axes

jona-sassenhagen · 2017-11-15T05:22:21Z

mne/viz/evoked.py

+    show_sensors: bool | int | str | None
        If not False, channel locations are plotted on a small head circle.
-        If an int, the position of the axes (forwarded to
+        If int or str, the position of the axes (forwarded to


The position of the head circle
or
The position of the head circle axes

jona-sassenhagen · 2017-11-15T05:27:23Z

mne/viz/evoked.py

+    # dealing with continuous colors
+    the_colors = None
+    color_conds = None
+    color_order = None


the_colors, color_conds, color_order = None, None, None

jona-sassenhagen · 2017-11-15T05:29:27Z

mne/evoked.py

+def _check_evokeds_ch_names_times(all_evoked):
+    evoked = all_evoked[0]
+    ch_names = evoked.ch_names
+    for e in all_evoked[1:]:


single-char var name ...

jona-sassenhagen · 2017-11-15T05:30:57Z

tutorials/plot_metadata_epochs.py

+metadata = epochs.metadata
+is_concrete = metadata["Concreteness"] > metadata["Concreteness"].median()
+metadata["is_concrete"] = np.where(is_concrete, 'Concrete', 'Abstract')
+is_concrete = metadata["NumberOfLetters"] > 5


jona-sassenhagen · 2017-11-15T05:31:35Z

Thanks Alex, that looks very clean.

ktavabi · 2017-11-15T07:01:05Z

mne/viz/evoked.py

+    gradiometers are combined with RMSE. It means for example that for
+    a vectorview system with 204 gradiometers it will be transformed to
+    a 102 channels helmet.



For example/E.g., data from a Vectorview system with 204 gradiometers will be transformed to 102 channels.

ktavabi · 2017-11-15T07:02:53Z

mne/viz/evoked.py

+        gradiometer corresponding pairs with be combined.
+        If None, it defaults to all data channels, in which case the global
+        field power will be plotted for all channel type available.
    gfp : bool


If the selected channels are gradiometers, the signal from corresponding (gradiometer) pairs will be combined. If None, it defaults to all data channels, in which case the global field power will be plotted for all available channel types.

ktavabi · 2017-11-15T07:06:23Z

mne/viz/evoked.py

+    vlines : "auto" | list of float
+        A list in seconds at which to plot dashed vertical lines.
+        If "auto" and 0. is in the time interval of interest, it is set to [0.]
+        so a vertical bar is plotted at time 0.


If "auto" and 0. ms is the time point of interest, it is set to [0.] and a vertical bar is plotted at time 0.

ktavabi · 2017-11-15T07:14:57Z

mne/viz/evoked.py

    fig : Figure | list of Figures
-        The figure(s) in which the plot is drawn.
+        The figure(s) in which the plot is drawn. A list of figures
+        is returned only one multiple channel types are plotted.


When plotting multiple channel types, a list of figures, one for each channel type is returned.

ktavabi · 2017-11-15T07:27:44Z

Thanks @agramfort

larsoner · 2017-11-15T14:16:13Z

mne/viz/evoked.py

+    When multiple channels are passed, this function combines them all, to
+    get one time course for each condition. If gfp is True it combines
+    channels using global field power (GFP) computation, else it is taking
+    a plain mean.


this preamble is 3 paragraphs, can we put this into Notes instead?

larsoner · 2017-11-15T14:16:59Z

mne/viz/evoked.py

+    GFP across datasets. It can also compute some confidence intervals
+    using for example bootstrap estimates.
+
+    When passed with more than one planar gradiometers, the planar


When ``picks`` includes more than one planar gradiometer, ...

larsoner · 2017-11-15T14:17:16Z

mne/viz/evoked.py

+    When passed with more than one planar gradiometers, the planar
+    gradiometers are combined with RMSE. It means for example that for
+    a vectorview system with 204 gradiometers it will be transformed to
+    a 102 channels helmet.


102-channel helmet

larsoner · 2017-11-15T14:18:28Z

tutorials/plot_visualize_evoked.py

 # The plot is styled with dictionary arguments, again using "/"-separated tags.
 # We plot a MEG channel with a strong auditory response.
+#
+# For move advanced plotting using :func:`mne.viz.plot_compare_evokeds`


larsoner · 2017-11-15T14:19:19Z

mne/viz/evoked.py

+
+    This function is useful for comparing ER[P/F]s at a specific location.
+
+    It can plot a simple Evoked object or if supplied with a list or a dict


:class:`mne.Evoked`

larsoner · 2017-11-15T14:27:18Z

Seems to fix the problem for me. It would be good to add this test (I came up with this to help @ktavabi test his PR):

import numpy as np
from numpy.testing import assert_allclose
import matplotlib.pyplot as plt
import os.path as op
import mne

data_path = mne.datasets.testing.data_path()
fname_evoked = op.join(data_path, 'MEG', 'sample', 'sample_audvis-ave.fif')

rng = np.random.RandomState(0)
evoked = mne.read_evokeds(fname_evoked)[0]
evokeds = [evoked.copy() for _ in range(10)]
for evoked in evokeds:
    evoked.data += (rng.randn(*evoked.data.shape) *
                    np.std(evoked.data, axis=-1, keepdims=True))
for picks in ([0], [1], [2], [0, 2], [1, 2], [0, 1, 2],):
    figs = mne.viz.plot_compare_evokeds([evokeds], picks=picks, ci=0.95)
    if not isinstance(figs, list):
        figs = [figs]
    for fig in figs:
        ext = fig.axes[0].collections[0].get_paths()[0].get_extents()
        xs, ylim = ext.get_points().T
        assert_allclose(xs, evoked.times[[0, -1]])
        line = fig.axes[0].lines[0]
        xs = line.get_xdata()
        assert_allclose(xs, evoked.times)
        ys = line.get_ydata()
        assert (ys < ylim[1]).all()
        assert (ys > ylim[0]).all()
    plt.close('all')

jona-sassenhagen · 2017-11-17T14:52:25Z

@agramfort want me to directly commit to the branch?

codecov · 2017-11-17T14:55:49Z

Codecov Report

Merging #4755 into master will increase coverage by 0.01%.
The diff coverage is 92.83%.

@@            Coverage Diff             @@
##           master    #4755      +/-   ##
==========================================
+ Coverage   87.86%   87.87%   +0.01%     
==========================================
  Files         349      349              
  Lines       65819    66079     +260     
  Branches    11319    11383      +64     
==========================================
+ Hits        57829    58070     +241     
- Misses       5117     5130      +13     
- Partials     2873     2879       +6

agramfort · 2017-11-17T14:56:18Z

I started to address some comments. please take over if you have a bit of time over the next couple of days

jona-sassenhagen · 2017-11-17T23:16:45Z

Great, I'll try to get a bit of work done!

jona-sassenhagen · 2017-11-18T00:12:27Z

Pushed a few lines directly to your branch, I will continue tomorrow.

jona-sassenhagen · 2017-11-18T13:02:03Z

@larsoner I added your test.

jona-sassenhagen · 2017-11-18T14:24:39Z

Green ...

agramfort · 2017-11-18T14:30:02Z

mne/viz/evoked.py

+            raise ValueError('If evokeds is a dict and a cmap is passed, '
+                             'you must specify the colors.')
+            # XXX : I am a bit concerned about the duplication of
+            # the colors and cmap parameters.


@jona-sassenhagen what do you think about this?
we don't use colors anywhere in doc and examples and it has a non-trivial interaction with cmap.

thoughts?

Hm? Maybe I misunderstand, but colors is all over the example.

colors specifies the mapping from conditions to line colors relatively to the color map, cmap specifies the colormap (and the name for the colorbar). I see two alternatives, both of which we ruled out:

use magic to read relative line color from the condition names (my original idea, which you ruled out)

demand colors values to be RGB or RGBA (possible, but requires more user work)

Essentially, option two is viable, but would require the user do something like

import seaborn as sns from sklearn.preprocessing import QuantileTransformer n_bins = 10 qt = QuantileTransformer(n_bins).fit(epochs.metadata[cond]) colorscale = sns.color_palette("viridis", n_bins) colors = dict() for ... in ...: colors[xxx] = colorscale[qt.transform(...)]

every time they run the function.

ok ok. forget it.

should the comment be removed, then?

agramfort · 2017-11-20T13:28:16Z

good to go from my end.

larsoner

Otherwise LGTM

larsoner · 2017-11-20T14:42:10Z

mne/evoked.py

+    ch_names = evoked.ch_names
+    for ev in all_evoked[1:]:
+        assert ev.ch_names == ch_names, ValueError(
+            "%s and %s do not contain " "the same channels" % (evoked, ev))


assert statements are skipped with python -O, if this is meant to actually prevent some user error (rather than internal consistency check) this needs to be done the standard way

larsoner · 2017-11-20T14:43:06Z

mne/viz/evoked.py

 from numbers import Integral

 import numpy as np
+import matplotlib.lines as mlines


it probably doesn't matter for the backend choosing because you didn't import pyplot, but let's nest this one, too, to keep mne import times down

Ah I see, you told me to put it at the top of the function, not the file. So that's what I'll do.

larsoner · 2017-11-20T14:45:10Z

mne/viz/evoked.py

+            raise ValueError('If evokeds is a dict and a cmap is passed, '
+                             'you must specify the colors.')
+            # XXX : I am a bit concerned about the duplication of
+            # the colors and cmap parameters.


should the comment be removed, then?

larsoner · 2017-11-20T14:46:02Z

mne/viz/evoked.py

+        - a list or dict of :class:`mne.Evoked` objects (e.g., for multiple
+          conditions),
+        - a list or dict of lists of :class:`mne.Evoked` (e.g., for multiple
+          subjects in multiple conditions).


no need to indent, doing so turns it from a standard bullet list to a quoted bullet list

larsoner · 2017-11-20T14:46:27Z

mne/viz/evoked.py

-    if (tmin >= 0 or tmax <= 0) and vlines == [0.]:
-        vlines = list()
+
+    if vlines is "auto" and (tmin < 0 and tmax > 0):


don't use is with string comparisons

larsoner · 2017-11-20T14:47:24Z

mne/viz/evoked.py

+
+    if vlines is "auto" and (tmin < 0 and tmax > 0):
+        vlines = [0.]
+    assert isinstance(vlines, list)


From the code above I don't think you've validated that it should be a list at this point, so this should be a full if not isinstance(vlines, (list, tuple)): raise TypeError(...)

larsoner · 2017-11-20T14:48:33Z

mne/viz/evoked.py

+        if not isinstance(axes, list):
+            axes = [axes]
+        from .utils import _validate_if_list_of_axes
+        _validate_if_list_of_axes(axes, obligatory_len=len(ch_types))


why not axes = _validate_if_list_of_axes(...) and do the if not isinstance(...) in that function? It seems like part of the validation

I've decided against making that change here cause the function is somewhat widely used, mostly in contexts I don't know. I can do it in a separate PR if you still think it should be done (assign me if you want).

let's get it in a subsequent PR if we find time

larsoner · 2017-11-20T14:49:23Z

mne/viz/evoked.py

-    # XXX: could possibly be refactored; plot_joint is doing a similar thing
-    if any([type_ not in _VALID_CHANNEL_TYPES for type_ in ch_types]):
-        raise ValueError("Non-data channel picked.")
+        axes = []


Could do instead:

axes = [plt.subplots(figsize=(8, 6))[1] for _ in range(len(ch_types))]

larsoner · 2017-11-20T14:51:12Z

mne/viz/evoked.py

+    # calculate the CI
+    ci_dict = dict()
+    data_dict = dict()
+    for cond, this_evokeds in evokeds.items():


order here is not deterministic, probably better to do for cond in sorted(evokeds.keys()): this_evokeds = evokeds[key] ...

larsoner · 2017-11-20T14:52:42Z

mne/viz/tests/test_evoked.py


 warnings.simplefilter('always')  # enable b/c these tests throw warnings

+rng = np.random.RandomState(0)


better to nest this in the function that uses it, lest the next contributor add a function and think it's okay to use the same rng (it could break your test with a heisenbug)

jona-sassenhagen · 2017-11-20T16:55:06Z

Thanks Eric, I will take care of things.

On 20. Nov 2017, at 15:53, Eric Larson ***@***.***> wrote: it probably doesn't matter for the backend choosing because you didn't import pyplot, but let's nest this one, too, to keep mne import times down

I’m pretty sure *you* told me to un-nest it :D

larsoner · 2017-11-20T16:58:42Z

I suppose that means we've asymptoted to a good result :)

Change it or not, then, up to you

agramfort · 2017-11-20T20:40:52Z

@jona-sassenhagen are you on it?

jona-sassenhagen · 2017-11-20T20:41:47Z

Yes, I will try to get it done today.

jona-sassenhagen · 2017-11-21T08:44:17Z

Green.

agramfort · 2017-11-21T08:53:37Z

@larsoner merge if you're happy

larsoner

One small remaining problem then +1 for merge

larsoner · 2017-11-21T14:21:12Z

mne/evoked.py

+    evoked = all_evoked[0]
+    ch_names = evoked.ch_names
+    for ev in all_evoked[1:]:
+        if not ev.ch_names == ch_names:


more easily expressed as ev.ch_names != ch_names but you can keep it this way

larsoner · 2017-11-21T14:21:41Z

mne/viz/evoked.py

+    if vlines == "auto" and (tmin < 0 and tmax > 0):
+        vlines = [0.]
+    if not isinstance(vlines, (list, tuple)):
+        raise TypeError("vlines must be a list or tuple, not " + type(vlines))


This must not be covered by a test because it will error, you should use % as we usually do:

In [1]: 'a' + type('b') Traceback (most recent call last): File "<ipython-input-1-03514d031c8f>", line 1, in <module> 'a' + type('b') TypeError: must be str, not type

larsoner · 2017-11-21T14:23:34Z

mne/viz/evoked.py

+    if axes is not None:
+        if not isinstance(axes, list):
+            axes = [axes]
+        from .utils import _validate_if_list_of_axes


this shouldn't need to be nested, in fact there is already a from .utils import ... line at the top

jona-sassenhagen · 2017-11-21T19:41:54Z

@ktavabi you are happy too?

ktavabi · 2017-11-21T22:56:58Z

I've been happy since the takeover. Thanks. Just waiting to get back on master :)

larsoner · 2017-11-22T06:24:26Z

Thanks @agramfort @ktavabi @jona-sassenhagen

jona-sassenhagen · 2017-11-22T09:11:55Z

Thanks guys!

* [ENH] plot_compare_evoked improved line colouring * FIX plot_compared_evokeds grad ci estimation * Compute stats after computing RMS * reorg + simplify grad handling of plot_compare_evokeds * use auxilary functions * add exception and test for corner case with both cmap and colors * address comments * cosmit, docstrings * pep8 * add erics test for grad cis * docstring * comments * pep8 :| * comments

ktavabi and others added 6 commits November 15, 2017 06:10

FIX plot_compared_evokeds grad ci estimation

0bd7f49

Compute stats after computing RMS

46c4e6b

reorg + simplify grad handling of plot_compare_evokeds

e7df71f

[ENH] plot_compare_evoked improved line colouring

9a0aaf5

use auxilary functions

e863f9a

add exception and test for corner case with both cmap and colors

0ee9c92

jona-sassenhagen reviewed Nov 15, 2017

View reviewed changes

ktavabi reviewed Nov 15, 2017

View reviewed changes

agramfort mentioned this pull request Nov 15, 2017

[DO NOT MRG] plot_compare_evoked improved line colouring #4526

Closed

larsoner reviewed Nov 15, 2017

View reviewed changes

address comments

451f705

cosmit, docstrings

af4e181

jona-sassenhagen added 2 commits November 18, 2017 13:50

pep8

5192e2a

add erics test for grad cis

f1b4ef1

docstring

dbfd361

agramfort commented Nov 18, 2017

View reviewed changes

agramfort changed the title ~~Fix compare evokeds~~ [MRG] Fix compare evokeds Nov 20, 2017

larsoner reviewed Nov 20, 2017

View reviewed changes

jona-sassenhagen mentioned this pull request Nov 20, 2017

MRG Simpler regression example #4765

Merged

jona-sassenhagen added 2 commits November 21, 2017 01:10

comments

c45315d

pep8 :|

a789d90

larsoner reviewed Nov 21, 2017

View reviewed changes

comments

5820eb8

larsoner approved these changes Nov 21, 2017

View reviewed changes

larsoner merged commit 44bac44 into mne-tools:master Nov 22, 2017



		def _check_loc_legal(loc, what='your choice'):
		"""Check if a loc is a legal for MPL."""



		def _format_evokeds_colors(evokeds, cmap, colors):
		# set up labels and instances to have evokeds as a dict


		This function is useful for comparing ER[P/F]s at a specific location.

		It can plot a simple Evoked object or if supplied with a list or a dict


		warnings.simplefilter('always') # enable b/c these tests throw warnings

		rng = np.random.RandomState(0)

Uh oh!

[MRG] Fix compare evokeds #4755

[MRG] Fix compare evokeds #4755

Uh oh!

Conversation

agramfort commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jona-sassenhagen commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktavabi Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktavabi Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktavabi commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Nov 15, 2017

Uh oh!

jona-sassenhagen commented Nov 17, 2017

Uh oh!

codecov bot commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agramfort commented Nov 17, 2017 via email

Uh oh!

jona-sassenhagen commented Nov 17, 2017

Uh oh!

jona-sassenhagen commented Nov 18, 2017

Uh oh!

jona-sassenhagen commented Nov 18, 2017

Uh oh!

jona-sassenhagen commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Nov 20, 2017

ktavabi Nov 15, 2017 •

edited

Loading

ktavabi Nov 15, 2017 •

edited

Loading

codecov bot commented Nov 17, 2017 •

edited

Loading