Skip to content

Conversation

larsoner
Copy link
Member

Closes #3702.

This PR:

  1. Unifies scale->scalings
  2. Fixes lots of warnings in tests
  3. Includes a fix for plot_topo that was necessary for tests to pass locally (and for interactivity to work for me at all), so I slightly modified two tutorials so that CircleCI would generate them.

Ready for review/merge from my end.

(One of the last big blockers for 0.15!)

@jona-sassenhagen
Copy link
Contributor

Unrelated, but where can I find the circle renderings for a build?

@larsoner larsoner added this to the 0.15 milestone Sep 22, 2017
@larsoner
Copy link
Member Author

"Details" of CircleCI on this page, sign in with GitHub, "Artifacts" tab

@codecov-io
Copy link

Codecov Report

Merging #4595 into master will decrease coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #4595      +/-   ##
==========================================
- Coverage   87.04%   87.02%   -0.02%     
==========================================
  Files         349      349              
  Lines       66030    66073      +43     
  Branches    10251    10254       +3     
==========================================
+ Hits        57476    57501      +25     
- Misses       5661     5675      +14     
- Partials     2893     2897       +4

@agramfort
Copy link
Member

LGTM

any other reviewer? @dengemann @choldgraf @jasmainak ?

@agramfort
Copy link
Member

ping also @kingjr as you opened the original issue

scale : dict | float | None
Scale the data for plotting. If None, defaults to 1e6 for eeg, 1e13
for grad and 1e15 for mag.
scalings : dict | float | None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can scalings also be 'auto' here, or is that just for the raw plot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, I think it's unique to raw currently

for grad and 1e15 for mag.
scalings : dict | float | None
The scalings of the channel types to be applied for plotting.
If None, defaults to ``dict(eeg=1e6, grad=1e13, mag=1e15)``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to add a scale: ... DEPRECATED bit in the docstring?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we usually don't list deprecated params

time_viewer.canvas.key_press_event('ctrl+right')
time_viewer.canvas.key_press_event('left')
time_viewer = fig.time_viewer
_fake_click(time_viewer, time_viewer.axes[0], (0.5, 0.5)) # change t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol did you do this just to avoid the pep monster?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

import matplotlib.pyplot as plt
if show and matplotlib.get_backend() != 'agg':
plt.show(**kwargs)
(fig or plt).show(**kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wtf is this python voodoo...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python has a short-circuit functionality. So if fig evaluates to True (in this case if it's a Figure instead of the default None) it will use it, otherwise it will use plt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is magical

@choldgraf
Copy link
Contributor

a few small comments, but looks good. +1000 to API standardization

@larsoner larsoner requested a review from kingjr September 24, 2017 19:46
@agramfort agramfort merged commit d12520c into mne-tools:master Sep 25, 2017
@agramfort
Copy link
Member

thx @larsoner

@larsoner larsoner deleted the scalings branch September 25, 2017 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: plot_topomap, plot_joint & plot: API homogeneity for scalings/scale parameters
5 participants