MRG Default ica.plot_properties to a parametric CI #6270

jona-sassenhagen · 2019-05-05T12:30:39Z

I am getting approx. half a second speedup for the tests on my machine.

Note I changed the functionality of plot_compare_evokeds a bit: now, as the doctstring already said, it will skip the CI if any condition has fewer than 2 evokeds.

@mmagnuski

* 'master' of git://github.com/mne-tools/mne-python: (48 commits) Update brainvision.py FIX: bv read vmrks NaN duration FIX: Fix for new numpy FIX: Minor fixes Updated test. Bug fix misc FIX: Fix docs FIX: Fix docstring FIX: Fix rendering [ci skip] Address comments Update whats_new sfreq and whats new ADD v2 hdr/mrk ENH header check, default event_id=None ADD event_id arg ENH helper function for version ENH add channel position via montage FIX potential fix to scaling issue ENH support v2.0 ...

codecov · 2019-05-05T12:43:31Z

Codecov Report

Merging #6270 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #6270      +/-   ##
==========================================
+ Coverage   89.37%   89.38%   +<.01%     
==========================================
  Files         416      416              
  Lines       74975    74983       +8     
  Branches    12328    12329       +1     
==========================================
+ Hits        67007    67020      +13     
+ Misses       5133     5129       -4     
+ Partials     2835     2834       -1

jona-sassenhagen · 2019-05-05T12:45:02Z

I also added this to plot_compare_evoked with axes='topo'. This speeds up the plot by a lot, e.g. 4fold for 30 channels! Not much for tests though.

Edit: seems I am getting half a second on the tests here too.

lgtm-com · 2019-05-05T13:28:10Z

This pull request introduces 1 alert when merging 8c857cc into 14a0f19 - view on LGTM.com

new alerts:

1 for Unused local variable

Comment posted by LGTM.com

mne/stats/parametric.py

mne/viz/evoked.py

mmagnuski · 2019-05-06T16:21:15Z

Looks good. How are the shades drawn for the psd? It's a different function for that, right?

jona-sassenhagen · 2019-05-06T16:28:58Z

Yeah and I think you wrote it :D

mmagnuski · 2019-05-06T17:14:45Z

That's right :) I was just trying to check whether psd stds were not refactored to be drawn via plot_compare_evokeds. I made sure by looking at the code now.
Then I don't have any other questions, LGTM. :)

mmagnuski · 2019-05-07T16:04:48Z

Travis and Azure complain.

mne/viz/evoked.py

jona-sassenhagen · 2019-05-10T21:30:07Z

Done I think

mne/viz/evoked.py

agramfort · 2019-05-11T08:39:06Z

mne/viz/ica.py

+        standard deviation above/below.
+        For the ERP/ERF, by default, plot the 95 percent parametric confidence
+        interval is calculated. To change this, use ``ci`` in ``ts_args`` in
+        ``image_args`` (see below).


why is this called ci at some place and std at others? When I read plot_std for me you report +/- 1 standard deviation on the plots. This needs to be made consistent.

Yes, for psd plot we first used +/-1 std but later when evoked plot was refactored to use plot_compare_evokeds it used 95% CI. So currently spectrum uses +/- 1 std and evoked plot uses CI.

I am not sure what would be best. The evoked reuses plot_compare_evokeds. We could use the one-sided CI functionality in both evoked and psd (and add it to `mne.stats._ci´), and call it a 95% CI for both.

@mmagnuski ?

agramfort · 2019-05-12T09:58:21Z

so anything to fix / improve for consistency / clarity?

…

mne/viz/evoked.py

agramfort · 2019-05-12T19:04:24Z

my suggestions are not clear @jona-sassenhagen ?

agramfort · 2019-05-12T19:10:42Z

before thinking about code changes, what would be a consistent API for all functions that can display a confidence interval / uncertainty on an average?

larsoner · 2019-05-14T19:42:39Z

@jona-sassenhagen do you think we can sort this out in the next couple of days to get it in for 0.18, or should we move the milestone to 0.19?

jona-sassenhagen · 2019-05-14T19:43:33Z

The brainvision reader is IMO more important than this one, so I don't mind moving this to .19.

agramfort · 2019-07-06T12:15:56Z

what's left to do here @jona-sassenhagen ?

jona-sassenhagen · 2019-07-06T19:37:09Z

i'd like to wait for #6422.

jona-sassenhagen · 2019-07-21T20:33:50Z

##[section]Starting: Run tests
==============================================================================
Task         : Command line
Description  : Run a command line script using Bash on Linux and macOS and cmd.exe on Windows
Version      : 2.151.1
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/command-line
==============================================================================
Generating script.
Script contents:
pytest -m "not ultraslowtest" mne
========================== Starting Command Output ===========================
##[command]"C:\windows\system32\cmd.exe" /D /E:ON /V:OFF /S /C "CALL "D:\a\_temp\02d3479f-eba7-4944-9140-065b5e3d239c.cmd""
'pytest' is not recognized as an internal or external command,
operable program or batch file.
##[error]Cmd.exe exited with code '1'.
##[section]Finishing: Run tests

What?

mmagnuski · 2019-07-21T22:48:05Z

I don't know if this is related but Azure here seems to have the same problem during installation as in this other PR, see here.
There it seems that fixing CONDA_VERSION to 4.7.5 in azure-pipelines.yml helped.

GuillaumeFavelier · 2019-07-22T08:56:18Z

I looked at the failing Windows Python37-64bit-full-conda build and at the end of the log of Install dependencies with conda, I see that you encounter the same error message as me:

LinkError: post-link script failed for package defaults::ipykernel-5.1.1-py37h39e3cac_0
location of failed script: D:\a\1\s\conda\Scripts\.ipykernel-post-link.bat
==> script messages <==
<None>
==> script output <==
stdout: 
stderr: '"D:\a\1\s\conda\condabin\conda.bat"' is not recognized as an internal or external command,

operable program or batch file.


return code: 1

()

larsoner · 2019-07-22T11:05:04Z

@jona-sassenhagen rebase and Azure should be okay. But we can also ignore it and trust Travis for this if you want

jona-sassenhagen · 2019-07-25T20:59:51Z

Rebase fixed this.

Further comments @drammock @agramfort @mmagnuski ?

drammock · 2019-07-26T01:31:12Z

code looks reasonable. I agree with @agramfort's point about ci vs std --- it would be good to unify handling of confidence bands. But (unless I'm missing something) I don't see that as a blocker for this PR.

agramfort · 2019-07-26T05:52:58Z

thx @jona-sassenhagen

* FIX to account for different versions of sphinx_gallery * FIX address comments * FIX : always return array in pick_types * FIX: Fix type * init * ... * fix docstring * ... * also for plot_compare_evokeds topomode * .. * fix test and docstrong * fixes * make tests green * pep8

jasmainak and others added 5 commits December 11, 2015 14:51

FIX to account for different versions of sphinx_gallery

4aa8c63

FIX address comments

7c53839

FIX : always return array in pick_types

a55460f

FIX: Fix type

a018846

jona-sassenhagen requested a review from mmagnuski May 5, 2019 12:32

jona-sassenhagen changed the title ~~Default ica.plot_properties to a parametric CI~~ MRG Default ica.plot_properties to a parametric CI May 6, 2019

mmagnuski reviewed May 6, 2019

View reviewed changes

mne/stats/parametric.py Show resolved Hide resolved

mmagnuski reviewed May 6, 2019

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

mmagnuski approved these changes May 6, 2019

View reviewed changes

larsoner reviewed May 7, 2019

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

agramfort requested changes May 11, 2019

View reviewed changes

jona-sassenhagen commented May 12, 2019

View reviewed changes

mne/viz/evoked.py Outdated Show resolved Hide resolved

larsoner added this to the 0.18 milestone May 13, 2019

larsoner modified the milestones: 0.18, 0.19 May 14, 2019

GuillaumeFavelier mentioned this pull request Jul 22, 2019

TST: Testing CIs for LinkError on Azure #6590

Merged

jona-sassenhagen added 11 commits July 22, 2019 13:05

Merge branch 'master' of 192.30.253.113:jona-sassenhagen/mne-python

654c369

init

2d4d328

...

8a025bf

fix docstring

11cb2ee

...

5e6cabf

also for plot_compare_evokeds topomode

4c6696f

..

dc78521

fix test and docstrong

45d5413

fixes

ecd8eb3

make tests green

7fe6f51

pep8

65c7e3f

jona-sassenhagen force-pushed the plot_properties_speed branch from 03e0e58 to 65c7e3f Compare July 22, 2019 11:22

agramfort merged commit f39331c into mne-tools:master Jul 26, 2019

Uh oh!

MRG Default ica.plot_properties to a parametric CI #6270

MRG Default ica.plot_properties to a parametric CI #6270

Uh oh!

Conversation

jona-sassenhagen commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jona-sassenhagen commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented May 5, 2019

Uh oh!

Uh oh!

Uh oh!

mmagnuski commented May 6, 2019

Uh oh!

jona-sassenhagen commented May 6, 2019

Uh oh!

mmagnuski commented May 6, 2019

Uh oh!

mmagnuski commented May 7, 2019

Uh oh!

Uh oh!

jona-sassenhagen commented May 10, 2019

Uh oh!

Uh oh!

Uh oh!

agramfort May 11, 2019

Choose a reason for hiding this comment

Uh oh!

mmagnuski May 12, 2019

Choose a reason for hiding this comment

Uh oh!

jona-sassenhagen May 12, 2019

Choose a reason for hiding this comment

Uh oh!

jona-sassenhagen Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

agramfort commented May 12, 2019 via email

Uh oh!

Uh oh!

agramfort commented May 12, 2019

Uh oh!

agramfort commented May 12, 2019 via email

Uh oh!

larsoner commented May 14, 2019

Uh oh!

jona-sassenhagen commented May 14, 2019

Uh oh!

agramfort commented Jul 6, 2019

Uh oh!

jona-sassenhagen commented Jul 6, 2019

Uh oh!

jona-sassenhagen commented Jul 21, 2019

Uh oh!

mmagnuski commented Jul 21, 2019

Uh oh!

GuillaumeFavelier commented Jul 22, 2019

Uh oh!

larsoner commented Jul 22, 2019

Uh oh!

jona-sassenhagen commented Jul 25, 2019

Uh oh!

drammock commented Jul 26, 2019

Uh oh!

agramfort commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jona-sassenhagen commented May 5, 2019 •

edited

Loading

codecov bot commented May 5, 2019 •

edited

Loading

jona-sassenhagen commented May 5, 2019 •

edited

Loading