[DO NOT MRG] plot_compare_evoked improved line colouring #4526

jona-sassenhagen · 2017-09-02T07:36:39Z

Redo of #4221

Good for parametric and complex factorial designs.

See gist

jona-sassenhagen · 2017-09-02T08:03:07Z

@mmagnuski can you eventually check if the API works for you? I remember your ICON poster

codecov-io · 2017-09-02T10:55:18Z

Codecov Report

Merging #4526 into master will decrease coverage by 0.06%.
The diff coverage is 84.95%.

@@            Coverage Diff             @@
##           master    #4526      +/-   ##
==========================================
- Coverage   87.85%   87.79%   -0.07%     
==========================================
  Files         349      349              
  Lines       65819    66194     +375     
  Branches    11319    11503     +184     
==========================================
+ Hits        57826    58112     +286     
- Misses       5119     5186      +67     
- Partials     2874     2896      +22

agramfort · 2017-09-02T11:22:24Z

I propose to complete this once we have metadata in + kword file in datasets. This gist will make an amazing tutorial ! really nice @jona-sassenhagen 👍

jona-sassenhagen · 2017-09-02T11:27:21Z

Yeah, makes sense.

We can in fact reproduce the full analysis in Dufau, Grainger, ... 2015 in a few lines of code with MNE. I'd like to demonstrate how to use the cluster correction, cause it seems to confuse many newcomers, but is very hip in ERPland these days.

agramfort · 2017-09-13T19:31:05Z

@jona-sassenhagen can you rebase?

what's left to do here?

jona-sassenhagen · 2017-09-13T19:31:35Z

Well you said:

I propose to complete this once we have metadata in + kword file in datasets

?

jona-sassenhagen · 2017-09-13T19:31:54Z

Or do you just mean the tutorial.

agramfort · 2017-09-13T19:34:07Z

my bad.... forget it...

larsoner · 2017-09-13T19:38:15Z

Yeah this is highly related to / mutually dependent on #4414, and we don't want that in before release. So we'll have to wait. But after we release, as far as I'm concerned this can go in first then we can simplify #4414

jona-sassenhagen · 2017-10-19T21:19:14Z

Eric, try the plot_metadata_epochs tutorial with this :)

larsoner · 2017-10-19T21:31:28Z

Can you rebase and touch the tutorial to make circle do the work?

jona-sassenhagen · 2017-10-19T21:32:14Z

I just did both - is there anything wrong?

jona-sassenhagen · 2017-10-19T22:02:45Z

Oh god, there are tons of errors.

larsoner · 2017-10-19T23:19:03Z

You need to make some change (even a trivial rewrite / typo fix) tutorials/plot_epochs_metadata.py otherwise Circle won't re-render it until merge, see:

https://6222-1301584-gh.circle-artifacts.com/0/tmp/circle-artifacts.1VuPluS/html/auto_tutorials/plot_metadata_epochs.html

jona-sassenhagen · 2017-10-19T23:21:21Z

Oh I see, I had those changes locally, but they were not pushed after the rebase (I assume because they were not changed by the original PR).

larsoner · 2017-10-19T23:50:54Z

Nice!

https://6223-1301584-gh.circle-artifacts.com/0/tmp/circle-artifacts.feoSvsz/html/auto_tutorials/plot_metadata_epochs.html

I would separate the last two plots into separate sections so they appear full-size

jona-sassenhagen · 2017-10-19T23:53:00Z

Ugh, these look terrible :D They're supposed to look like this:

larsoner · 2017-10-20T00:26:07Z

CircleCI doesn't have viridis, its matplotlib is too old

jona-sassenhagen · 2017-10-20T19:21:48Z

Oh wow, we are green, but I need to rebase from my own 2-line PR.

:|

agramfort · 2017-10-20T19:22:46Z

I am sure you will survive...

larsoner · 2017-10-20T19:28:36Z

@jona-sassenhagen once it comes back can you check CircleCI and post a link?

jona-sassenhagen · 2017-10-20T19:29:02Z

I will try :D

jona-sassenhagen · 2017-10-20T19:54:46Z

Hm. Something is wrong here.

jona-sassenhagen · 2017-10-20T19:56:55Z

I get the same for my local Python 2.7 :(

larsoner · 2017-10-20T20:02:02Z

Time to work on those debugging skills

jona-sassenhagen · 2017-10-20T22:09:43Z

Well - better.
https://6255-1301584-gh.circle-artifacts.com/0/tmp/circle-artifacts.dIS8NwY/html/auto_tutorials/plot_metadata_epochs.html

mmagnuski · 2017-11-13T21:01:09Z

BTW - is the legend a parasite axis (or something like this)? They are much harder to customize (change position etc.), I thinkI had this problem with plot_joint in the past.

jona-sassenhagen · 2017-11-13T21:02:53Z

It's an mpl_toolkits.axes_grid1.inset_locator.inset_axes (if you mean the channels), the function is plot_legend.

jona-sassenhagen · 2017-11-14T03:03:44Z

Oh nice. We are green.

jona-sassenhagen · 2017-11-14T03:04:09Z

Any chance we can get this merged before tomorrow morning? :)

larsoner · 2017-11-14T03:57:11Z

tutorials/plot_metadata_epochs.py

@@ -1,4 +1,6 @@
 """
+.. _tut_metadata_epochs:


don't add this, there is a standard ref tag automagically inserted based on filename sphx_glr_auto_tutorials_plot_metadata_epochs.py

Ugh, how did this end up there? Must have come with the rebase or something. I didn't do it manually.

jona-sassenhagen · 2017-11-14T03:59:33Z

tutorials/plot_visualize_evoked.py

 # We plot a MEG channel with a strong auditory response.
+#
+# For move advanced plotting using :func:`mne.viz.plot_compare_evokeds`
+# see also :ref:`tut_metadata_epochs`.


This too - I didn't put it here.

Should the line be changed? @larsoner

@agramfort did:

cfb5ce6

It's fine to leave the new sentences but remove the _tut_ ref and replace it with the standard one. You can check correctness locally with cd doc/; make html_dev-noplot, you should only see 5 (I think?) warnings about gallery images missing in manual/.

larsoner

Other than my minor gripes LGTM, but I don't fully appreciate/understand all the use cases, so @mmagnuski I'll rely on your judgment here

larsoner · 2017-11-14T03:58:15Z

mne/viz/evoked.py

+        if colors is None:
+            raise ValueError(
+                "If `split_legend` is True, `colors` must not be None.")
+        import matplotlib.lines as mlines


let's move this to the top of the function so the nested imports / namespace is clear

larsoner · 2017-11-14T03:59:54Z

mne/viz/evoked.py

+        ax_cb.set_xticks(())
+        ax_cb.set_ylabel(cmap_label)
+        fig.cbar = ax_cb
+    fig.ts_ax = axes


I would prefer to avoid figure.axes cause we have a varying number of axes.

But based on when it's created it should always be the last axis of the figure. It seems cleaner/more standard to get it as figure.axes[-1].

jona-sassenhagen · 2017-11-14T06:02:10Z

There, very green.

mmagnuski · 2017-11-14T10:56:55Z

@jona-sassenhagen I'll take a look this evening, be patient! :)

jona-sassenhagen · 2017-11-14T19:19:34Z

Thanks @mmagnuski !

mmagnuski · 2017-11-14T20:31:28Z

mne/viz/evoked.py

+    if ci is True:
+        ci = .95
+    elif ci is not False and not (isinstance(ci, np.float) or callable(ci)):
        raise TypeError('ci must be float or callable, got ' + str(type(ci)))


nitpick: "ci must be None, bool, float or callable ..."

Addressed, merge if you want.

mmagnuski · 2017-11-14T20:35:35Z

tutorials/plot_metadata_epochs.py

+for n_letters in letters:
+    evokeds[n_letters] = epochs["NumberOfLetters == " + n_letters].average()
+
+style_plot["colors"] = {n_letters: int(float(n_letters))


I like how colors are created now, as a side not the int(float(n_letters)) would not be necessary if NumberOfLetters in metadata was of int type.

Yes, I probably should have done it like that.

mmagnuski

Very cool, wholeheartedly approve!

mmagnuski · 2017-11-14T20:43:32Z

This already got at least +3 so far so I'll go ahead and merge.
(but I hope for float color option in the future :) - and my nitpicks could be adressed in this future PR if you agree @jona-sassenhagen)

jona-sassenhagen · 2017-11-14T22:06:10Z

Thanks everyone for reviews!

jona-sassenhagen · 2017-11-15T02:25:06Z

The travis build timed out ... restarted.

mmagnuski · 2017-11-15T13:03:44Z

codecov now complains, but previously all was green. My understanding of codecov is quite limited (I am often confused by why for the same PR one time it is green and another it compains) - so I refer to your intuitions @jona-sassenhagen @larsoner.

agramfort · 2017-11-15T13:34:46Z

@mmagnuski don't merge. I spend a significant of time yesterday to simplify this code.

see #4755

let me know If I miss some commits there and If none I'll close this PR

jona-sassenhagen mentioned this pull request Sep 2, 2017

Very WIP: sequential colors for plot_compare_evoked #4221

Closed

jona-sassenhagen mentioned this pull request Sep 13, 2017

MRG+4: Epochs metadata #4414

Merged

2 tasks

jona-sassenhagen force-pushed the plt_comp_ev_seq2 branch from e4e73cb to 73b4c10 Compare October 19, 2017 21:14

jona-sassenhagen force-pushed the plt_comp_ev_seq2 branch from 14b6b29 to cb7fd1c Compare October 20, 2017 19:24

jona-sassenhagen added 2 commits November 13, 2017 14:10

fix legend default

fd23602

*curses*

fb37f87

jona-sassenhagen added 2 commits November 13, 2017 15:23

fix plot_epochs_image

4c24b4d

pep

06a285a

jona-sassenhagen changed the title ~~[WIP] plot_compare_evoked improved line colouring~~ [MRG] plot_compare_evoked improved line colouring Nov 14, 2017

larsoner reviewed Nov 14, 2017

View reviewed changes

jona-sassenhagen commented Nov 14, 2017

View reviewed changes

larsoner approved these changes Nov 14, 2017

View reviewed changes

fixes

a5fc30a

mmagnuski reviewed Nov 14, 2017

View reviewed changes

mmagnuski approved these changes Nov 14, 2017

View reviewed changes

comment

b9afa02

jona-sassenhagen mentioned this pull request Nov 14, 2017

dedicated float option for mne.viz.plot_compare_evokeds #4753

Closed

agramfort mentioned this pull request Nov 15, 2017

[MRG] Fix compare evokeds #4755

Merged

agramfort changed the title ~~[MRG] plot_compare_evoked improved line colouring~~ [DO NOT MRG] plot_compare_evoked improved line colouring Nov 15, 2017

jona-sassenhagen closed this Nov 15, 2017

jona-sassenhagen deleted the plt_comp_ev_seq2 branch March 10, 2018 10:38

Uh oh!

[DO NOT MRG] plot_compare_evoked improved line colouring #4526

[DO NOT MRG] plot_compare_evoked improved line colouring #4526

Uh oh!

Conversation

jona-sassenhagen commented Sep 2, 2017

Uh oh!

jona-sassenhagen commented Sep 2, 2017

Uh oh!

codecov-io commented Sep 2, 2017 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agramfort commented Sep 2, 2017

Uh oh!

jona-sassenhagen commented Sep 2, 2017

Uh oh!

agramfort commented Sep 13, 2017

Uh oh!

jona-sassenhagen commented Sep 13, 2017

Uh oh!

jona-sassenhagen commented Sep 13, 2017

Uh oh!

agramfort commented Sep 13, 2017 via email

Uh oh!

larsoner commented Sep 13, 2017

Uh oh!

jona-sassenhagen commented Oct 19, 2017

Uh oh!

larsoner commented Oct 19, 2017 via email

Uh oh!

jona-sassenhagen commented Oct 19, 2017

Uh oh!

jona-sassenhagen commented Oct 19, 2017

Uh oh!

larsoner commented Oct 19, 2017

Uh oh!

jona-sassenhagen commented Oct 19, 2017 via email

Uh oh!

larsoner commented Oct 19, 2017

Uh oh!

jona-sassenhagen commented Oct 19, 2017

Uh oh!

larsoner commented Oct 20, 2017

Uh oh!

jona-sassenhagen commented Oct 20, 2017

Uh oh!

agramfort commented Oct 20, 2017 via email

Uh oh!

larsoner commented Oct 20, 2017

Uh oh!

jona-sassenhagen commented Oct 20, 2017

Uh oh!

jona-sassenhagen commented Oct 20, 2017

Uh oh!

jona-sassenhagen commented Oct 20, 2017

Uh oh!

larsoner commented Oct 20, 2017

Uh oh!

jona-sassenhagen commented Oct 20, 2017

Uh oh!

mmagnuski commented Nov 13, 2017

Uh oh!

jona-sassenhagen commented Nov 13, 2017

Uh oh!

jona-sassenhagen commented Nov 14, 2017

Uh oh!

jona-sassenhagen commented Nov 14, 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 left a comment

codecov-io commented Sep 2, 2017 •

edited by codecov bot

Loading