Fix plot surface #7079

christian-oreilly · 2019-11-18T21:49:31Z

Reference issue

What does this implement/fix?

Removed config_opts from the call to plot_source_estimates(). Some functionalities that were once served by config_opts may have been lost. I don't know what use case this parameter was previously fulfilling.
Fixed an issue with transparency. Now, if transparent has been set to None when calling plot_source_estimates(), it is set to True (the default for plot_source_estimates()) before calling brain.add_data(**kwargs).
Adding PySurfer in the dependencies.

christian-oreilly · 2019-11-19T05:07:07Z

Travis gives two errors:
1 - ModuleNotFoundError: No module named 'vtk'
2 - KeyError: 'Key "SUBJECTS_DIR" not found in the environment or in the mne-python [...]
These two errors seem to be associated with setting up continuous integration so that it manages gracefully the dependencies of PySurfer. Not sure how to handle these but I am sure some of you have experience with these issues given the overlap in developers involved in Python-MNE and PySurfer.

GuillaumeFavelier

Thank you for contributing to mne-python! I just had a few comments. I don't know about the SUBJECTS_DIR error but maybe @agramfort and @larsoner have some ideas.

setup.py

mne/tests/test_source_estimate.py

setup.py

larsoner · 2019-11-19T11:56:54Z

mne/tests/test_source_estimate.py

+    # This requires FreeSurfer to be installed, so that the subject
+    # fsaverage exists.
+    stc.plot_surface(views='lat', hemi='split', src=src,
+                     subject='fsaverage', colorbar=False)


you need to pass subjects_dir=subjects_dir here so that it's happy

codecov · 2019-11-21T15:40:15Z

Codecov Report

Merging #7079 into master will increase coverage by <.01%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##           master    #7079      +/-   ##
==========================================
+ Coverage   89.74%   89.75%   +<.01%     
==========================================
  Files         444      444              
  Lines       79275    79296      +21     
  Branches    12677    12681       +4     
==========================================
+ Hits        71145    71169      +24     
+ Misses       5339     5337       -2     
+ Partials     2791     2790       -1

agramfort · 2019-11-21T16:18:10Z

mne/viz/tests/test_3d.py

+    stc = MixedSourceEstimate(data, vertno, 0, 1)
+
+    # This requires FreeSurfer to be installed, so that the subject
+    # fsaverage exists.


this is not really true. You can have fsaverage in yoru subjects dir without install freesurfer. Just remove this line.

agramfort · 2019-11-21T16:18:43Z

mne/viz/tests/test_3d.py

  0.1250  0.750e-03  0.750e-03  0.750e-03  0.000  0.000  1.000
 """

+rng = np.random.RandomState(0)


@agramfort This code has been copied and modified from somewhere else in MNE. rng is used in test_mixed_sources_plot_surface() to generate random number. I could use np.random.seed(seed=0) and np.random.randn() instead, if you prefer. It has the advantage of not adding "global" code and having all this locally in test_mixed_sources_plot_surface. I am not sure about the advantages of using the Mersenne Twister pseudo-random number generator in numpy.random.RandomState. As I said, I just copied this code from elsewhere in MNE, without questionning much why this generator had been chosen.

Agreed better to have it than not. But better to nest it in the test function that actually uses it (clearer where it's used, and also no temptation to use it in other functions, which could change the result in the test you're running, test order can break things, etc.)

Can you nest the rng def inside the function where it's used?

That plus add an entry to the BUG section of doc/changes/latest.inc and we should be good to go

@larsoner Both are done by 9de82bc

christian-oreilly · 2019-11-22T19:39:09Z

The CI error is unrelated to this PR. It failed while reading and H5 file. If you have no further comments, I seem ready to merge.

doc/changes/latest.inc

Co-Authored-By: Eric Larson <larson.eric.d@gmail.com>

agramfort · 2019-12-08T12:54:54Z

@christian-oreilly can you rebase and fix conflicts. CIs should come back green again.

* Initial fix * Update mesh is not necessary * Choose to modify scale_factor as post-post-process * Add simple test for fix * Use mean of hemi's width

* BUG: Fix running subprocesses in Jupyter notebooks * FIX: Flake [ci skip]

* Adding use_scalebars as a parameter. * Fixed to adhere to mne-coding standards. * Fixing formatting issues. * Fixing show_scalebars in viz ica from the original naming of use_scalebars. * Update mne/viz/ica.py Co-Authored-By: Daniel McCloy <dan.mccloy@gmail.com> * Update mne/viz/raw.py Co-Authored-By: Daniel McCloy <dan.mccloy@gmail.com> * DOC: Correct version [ci skip]

* ir scheme * Squash commits * messed up my rebase * Address comments Alex * flake * implement filtering and write more readable math code * fix pydocstyle * High pass filter, remove upsampling, get 4 sources * TFMxnE > TF-MxNE + rm comment example

…used (mne-tools#7090) * BUG: Write out meas_date=None * FIX: Maybe better? * DOC: Document

…nce and CrossSpectralDensity (mne-tools#7029) * Add more capabilites to equalize_channels * Add unit tests for improved equalize_channels functionality * Fix docstring of equalize_channels * Add Info capabilities to equalize_channels * Fix Forward.pick_channels docstring * Update combine_evokeds unit test * Fix edge case of pick_channels_cov * Add dedicated pick_channels_cov unit test * Add copy parameter to equalize_channels * Make copy=False the default * Make grand_average also equalize TFR channels * Make copy=True the default in equalize_channels * PEP8 * Update docstring * Update what's new * Update unit tests * Update links to methods in whats_new * Spell out new supported object types * Fix links in whats_new again * Remove attribute links

* rename file * WIP tutorial overhaul [ci skip] * link targets * finish tutorial revisions * fix glossary ref * formatting/flow tweaks [skip travis] * fix codespell * address neurolaunch review comments

* Start refactoring of report * Fix figure_size bug * Fix undefined function bug * Remove unnecessary 3d functions from public API * Refactor _fig_to_img * Unify 3d backend testing * Fix 3d backend testing variable * Minor refactoring * Improve _check_3d_figure * TST: Trigger plot_make_report.py * TST: Trigger plot_make_report.py * Fix typo * Fix bug with figure not closed * TST: Trigger plot_make_report.py * Integrate reviews

* better artifact plots; better thumbnail choice * keep it pedagogical * change thumbnail [ci skip]

* MAINT: Make calculate_head_pos_ctf public * FIX: Doc * DOC: Fix * FIX: Docstring

* refactor _handle_event_colors * restore warning * fix tests * fix circle * add test * minor refactor

* remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds

agramfort · 2019-12-08T13:26:56Z

@christian-oreilly something bad happened in your rebase. I don't know how this happened.

I would have done

git checkout master
git pull upstream master
git checkout fix-plot-surface
git rebase master
# fix conficts
git rebase --continue
git push origin fix-plot-surface

agramfort · 2019-12-08T13:30:14Z

mne/viz/tests/test_3d.py


 @testing.requires_testing_data
 @requires_nibabel()
+@requires_pysurfer


why do you need pysurfer for a test that says use matplotlib?

If I remember well, there is no direct call to pysurfer in that function but it ended in the calling stack using a function requiring pysurfer and was crashing the test.

please revert this one

agramfort · 2019-12-08T13:30:25Z

mne/viz/tests/test_3d.py

 @testing.requires_testing_data
 @requires_dipy()
 @requires_nibabel()
+@requires_version('dipy', '0.10.1')


If I remember well, one of the functions called checks for this condition, so the test was failling because this check was not fullfilled on my system.

please revert this one

agramfort · 2019-12-08T13:30:58Z

mne/viz/_3d.py

        vertices = stc.vertices[hemi_idx]
        if len(data) > 0:
+            if transparent is None:
+                transparent = True


Again, if I remember well, brain.add_data(**kwargs) called later was failling with transparent set to None.

docstring says

transparent : bool | None
If True, use a linear transparency between fmin and fmid.
None will choose automatically based on colormap type.

can you see how it is done by other function that expose this parameter? thx

christian-oreilly · 2019-12-08T13:36:01Z

@agramfort Not sure what went wrong. I am not a git wiz. I used

git branch --set-upstream-to=origin/master fix-plot-surface 
git pull --rebase
# fix conflict
git rebase --continue
# The push in the banch was failing, so I further did
git pull christian-fork fix-plot-surface  --rebase
git rebase --continue
# fix conflict
git push christian-fork

agramfort · 2019-12-08T13:46:13Z

please revert these changes to see if CIs complain

christian-oreilly · 2019-12-08T13:52:30Z

@agramfort What change are you refering to specifically? The rebase?

Co-Authored-By: Eric Larson <larson.eric.d@gmail.com>

* BUG: CTF - set meas_date * Comment: not to revisit testing post potential mne-c updates * Whats New

* remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds

christian-oreilly · 2019-12-08T15:22:27Z

@agramfort All green!

agramfort · 2019-12-08T13:55:47Z

mne/viz/tests/test_3d.py


 @testing.requires_testing_data
 @requires_nibabel()
+@requires_pysurfer


please revert this one

agramfort · 2019-12-08T13:56:03Z

mne/viz/tests/test_3d.py

 @testing.requires_testing_data
 @requires_dipy()
 @requires_nibabel()
+@requires_version('dipy', '0.10.1')


please revert this one

agramfort · 2019-12-08T13:59:34Z

mne/viz/_3d.py

        vertices = stc.vertices[hemi_idx]
        if len(data) > 0:
+            if transparent is None:
+                transparent = True


docstring says

transparent : bool | None
If True, use a linear transparency between fmin and fmid.
None will choose automatically based on colormap type.

can you see how it is done by other function that expose this parameter? thx

agramfort · 2019-12-08T18:06:20Z

doc/changes/latest.inc

+
+- Fix incorrect scaling of cluster temporal extent :func:`mne.stats.summarize_clusters_stc` by `Daniel McCloy`_.
+
+- Fix :meth:`mne.io.read_raw_ctf` to set measurement date from CTF ds files by `Luke Bloy`_.


@christian-oreilly the 2 items above are irrelevant here

agramfort · 2019-12-14T14:52:15Z

thx @christian-oreilly

the transparent issue is fine. Our code is not DRY but it works

* Fixing issues with plot_surface() * Setting transparent to True instead of False. * PR review edits. * PR review edits. * Improved error management for missing pysufer. * Update _3d.py * Update test_3d.py * Fixing test requires conditions. * Document this PR change. * Update doc/changes/latest.inc Co-Authored-By: Eric Larson <larson.eric.d@gmail.com> * fix : don't require git to install master (mne-tools#7078) * [MRG] Fix plot_vector_source_estimates (mne-tools#7084) * Initial fix * Update mesh is not necessary * Choose to modify scale_factor as post-post-process * Add simple test for fix * Use mean of hemi's width * MRG, BUG: Fix running subprocesses in Jupyter notebooks (mne-tools#7086) * BUG: Fix running subprocesses in Jupyter notebooks * FIX: Flake [ci skip] * FIX: Adding use_scalebars as a parameter. (mne-tools#7091) * Adding use_scalebars as a parameter. * Fixed to adhere to mne-coding standards. * Fixing formatting issues. * Fixing show_scalebars in viz ica from the original naming of use_scalebars. * Update mne/viz/ica.py Co-Authored-By: Daniel McCloy <dan.mccloy@gmail.com> * Update mne/viz/raw.py Co-Authored-By: Daniel McCloy <dan.mccloy@gmail.com> * DOC: Correct version [ci skip] * [MRG] Implement iterative reweighted TF-MxNE (mne-tools#5075) * ir scheme * Squash commits * messed up my rebase * Address comments Alex * flake * implement filtering and write more readable math code * fix pydocstyle * High pass filter, remove upsampling, get 4 sources * TFMxnE > TF-MxNE + rm comment example * MAINT: Ensure that meas_date=None round-trips if `anonymize_info` is used (mne-tools#7090) * BUG: Write out meas_date=None * FIX: Maybe better? * DOC: Document * [WIP] Make equalize_channels capable of operating on Forward, Covariance and CrossSpectralDensity (mne-tools#7029) * Add more capabilites to equalize_channels * Add unit tests for improved equalize_channels functionality * Fix docstring of equalize_channels * Add Info capabilities to equalize_channels * Fix Forward.pick_channels docstring * Update combine_evokeds unit test * Fix edge case of pick_channels_cov * Add dedicated pick_channels_cov unit test * Add copy parameter to equalize_channels * Make copy=False the default * Make grand_average also equalize TFR channels * Make copy=True the default in equalize_channels * PEP8 * Update docstring * Update what's new * Update unit tests * Update links to methods in whats_new * Spell out new supported object types * Fix links in whats_new again * Remove attribute links * MRG, DOC: revise tutorial: overview of Epoching (mne-tools#7011) * rename file * WIP tutorial overhaul [ci skip] * link targets * finish tutorial revisions * fix glossary ref * formatting/flow tweaks [skip travis] * fix codespell * address neurolaunch review comments * [MRG] Refactor mayavi/mlab out report (mne-tools#7008) * Start refactoring of report * Fix figure_size bug * Fix undefined function bug * Remove unnecessary 3d functions from public API * Refactor _fig_to_img * Unify 3d backend testing * Fix 3d backend testing variable * Minor refactoring * Improve _check_3d_figure * TST: Trigger plot_make_report.py * TST: Trigger plot_make_report.py * Fix typo * Fix bug with figure not closed * TST: Trigger plot_make_report.py * Integrate reviews * crop raw to save memory (mne-tools#7100) * MRG, DOC: Update contrib guide (mne-tools#7097) * WIP update contributing guide * refine bug reporting guidelines * add git setup diagram * mention dangers of PYTHONPATH * fix typo * more on PYTHONPATH * clarify dev environment setup steps * tweaks * remove self-referential crossref * add "origin" * ENH: Adding support of GridSearchCV for mne.decoding.LinearModel (mne-tools#7089) * ENH: Adding support of GridSearchCV for mne.decoding.LinearModel * (1) adds the GridSearchCV tests for LinearModel class. (2) adds Classification/Regression/GridSearchCV estimators' tests for get_coef function. * fixed some flake8 errors. * speeds up the tests. * tweaks some tests. * adds an entry to changelog. * (1) generates the random values under RandomState. (2) uses `with block` to catch exceptions. * BUG: CTF - set meas_date (mne-tools#7104) * BUG: CTF - set meas_date * Comment: not to revisit testing post potential mne-c updates * Whats New * MRG, DOC: revise tutorial: epochs visualization (mne-tools#7102) * delete redundant tutorial * add explanation of dropped epoch & crossref to drop_log section * add crossref target * overhaul epochs viz tutorial * fix sidebar * avoid warning * update crossrefs * formatting & flow tweaks * rename file for proper ordering * fix crossrefs * make plots look better * fix codespell * [MRG] Improve Renderer API (mne-tools#6761) * Fix minor bug in sphere() * Fix dimension in tube() * Update doc for color * Add color management as str * Find better default value for tube() radius * Correct colorConvertex syntax * Fix import nesting * Refactor colro code * Rework text2d() (x,y) parameter * Refactor _parse_str_color to _check_color * Add some tests * Improve doc * Add the radius parameter for pyvista * Fix import * FIX: Avoid deadlocking (mne-tools#7103) * DOC: Spelling (mne-tools#7106) * ENH: Add mne sys_info command (mne-tools#7105) * ENH: Add mne sys_info command * FIX: Install * MRG, DOC: tutorial tweaks (mne-tools#7109) * better artifact plots; better thumbnail choice * keep it pedagogical * change thumbnail [ci skip] * MAINT: Test on 3.8 (mne-tools#7113) * better info message for rank computation [skip circle] (mne-tools#7110) * FIX: Fix for old build (mne-tools#7120) * MRG, MAINT: Make calculate_head_pos_ctf public (mne-tools#7117) * MAINT: Make calculate_head_pos_ctf public * FIX: Doc * DOC: Fix * FIX: Docstring * MRG, FIX: Fix sklearn import (mne-tools#7121) * ENH: refactor _handle_event_colors (mne-tools#7111) * refactor _handle_event_colors * restore warning * fix tests * fix circle * add test * minor refactor * FIX: Allow unknown (mne-tools#7119) * MRG, FIX: scaling in summarize_clusters_stc (mne-tools#7125) * remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds * Document this PR change. * Document this PR change. * Update doc/changes/latest.inc Co-Authored-By: Eric Larson <larson.eric.d@gmail.com> * BUG: CTF - set meas_date (mne-tools#7104) * BUG: CTF - set meas_date * Comment: not to revisit testing post potential mne-c updates * Whats New * MRG, FIX: scaling in summarize_clusters_stc (mne-tools#7125) * remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds * Fix doc CI warning about missing target. * more * nest check

christian-oreilly added 2 commits November 18, 2019 16:27

Fixing issues with plot_surface()

918283e

Setting transparent to True instead of False.

74d4b35

christian-oreilly mentioned this pull request Nov 18, 2019

mne.source_estimate.MixedSourceEstimate.plot_surface(...) crashes #7015

Closed

GuillaumeFavelier reviewed Nov 19, 2019

View reviewed changes

setup.py Outdated Show resolved Hide resolved

mne/tests/test_source_estimate.py Outdated Show resolved Hide resolved

larsoner reviewed Nov 19, 2019

View reviewed changes

christian-oreilly added 2 commits November 21, 2019 10:34

PR review edits.

cd3e9ec

PR review edits.

ff86e88

christian-oreilly added 2 commits November 21, 2019 11:02

Improved error management for missing pysufer.

9015a62

Update _3d.py

2677f83

agramfort reviewed Nov 21, 2019

View reviewed changes

christian-oreilly added 2 commits November 22, 2019 10:58

Update test_3d.py

0c73789

Fixing test requires conditions.

c5231b7

Document this PR change.

9de82bc

larsoner reviewed Nov 22, 2019

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

Update doc/changes/latest.inc

ab936e4

Co-Authored-By: Eric Larson <larson.eric.d@gmail.com>

agramfort and others added 10 commits December 8, 2019 08:17

fix : don't require git to install master (mne-tools#7078)

7ea0b38

[MRG] Fix plot_vector_source_estimates (mne-tools#7084)

85851fc

* Initial fix * Update mesh is not necessary * Choose to modify scale_factor as post-post-process * Add simple test for fix * Use mean of hemi's width

MRG, BUG: Fix running subprocesses in Jupyter notebooks (mne-tools#7086)

268eea5

* BUG: Fix running subprocesses in Jupyter notebooks * FIX: Flake [ci skip]

MAINT: Ensure that meas_date=None round-trips if anonymize_info is …

7065da6

…used (mne-tools#7090) * BUG: Write out meas_date=None * FIX: Maybe better? * DOC: Document

MRG, DOC: revise tutorial: overview of Epoching (mne-tools#7011)

3c311ba

* rename file * WIP tutorial overhaul [ci skip] * link targets * finish tutorial revisions * fix glossary ref * formatting/flow tweaks [skip travis] * fix codespell * address neurolaunch review comments

crop raw to save memory (mne-tools#7100)

7d87a54

drammock and others added 11 commits December 8, 2019 08:18

MRG, DOC: tutorial tweaks (mne-tools#7109)

cf52ba5

* better artifact plots; better thumbnail choice * keep it pedagogical * change thumbnail [ci skip]

MAINT: Test on 3.8 (mne-tools#7113)

2fde180

better info message for rank computation [skip circle] (mne-tools#7110)

f96ba57

FIX: Fix for old build (mne-tools#7120)

f794309

MRG, MAINT: Make calculate_head_pos_ctf public (mne-tools#7117)

897e287

* MAINT: Make calculate_head_pos_ctf public * FIX: Doc * DOC: Fix * FIX: Docstring

MRG, FIX: Fix sklearn import (mne-tools#7121)

b209c6d

ENH: refactor _handle_event_colors (mne-tools#7111)

d2ffe4f

* refactor _handle_event_colors * restore warning * fix tests * fix circle * add test * minor refactor

FIX: Allow unknown (mne-tools#7119)

66e0a25

MRG, FIX: scaling in summarize_clusters_stc (mne-tools#7125)

3c1aa92

* remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds

Document this PR change.

6c1abbd

Merge branch 'master' into fix-plot-surface

d2e3be4

agramfort reviewed Dec 8, 2019

View reviewed changes

christian-oreilly and others added 5 commits December 8, 2019 09:23

Document this PR change.

f7eebc9

Update doc/changes/latest.inc

d7d2129

Co-Authored-By: Eric Larson <larson.eric.d@gmail.com>

BUG: CTF - set meas_date (mne-tools#7104)

82b89ca

* BUG: CTF - set meas_date * Comment: not to revisit testing post potential mne-c updates * Whats New

MRG, FIX: scaling in summarize_clusters_stc (mne-tools#7125)

1b9da84

* remove hardcoded 1e3; improve docstring * touch tutorials to trigger rendering * update what's new * actually use milliseconds

Fix doc CI warning about missing target.

ed1daa3

agramfort reviewed Dec 8, 2019

View reviewed changes

agramfort added 3 commits December 12, 2019 23:53

Merge branch 'master' into fix-plot-surface

77aef24

more

1556c4e

nest check

c63c518

agramfort merged commit 0c4c3b7 into mne-tools:master Dec 14, 2019


		- Fix incorrect scaling of cluster temporal extent :func:`mne.stats.summarize_clusters_stc` by `Daniel McCloy`_.

		- Fix :meth:`mne.io.read_raw_ctf` to set measurement date from CTF ds files by `Luke Bloy`_.

Uh oh!

Fix plot surface #7079

Fix plot surface #7079

Uh oh!

Conversation

christian-oreilly commented Nov 18, 2019

Reference issue

What does this implement/fix?

Uh oh!

christian-oreilly commented Nov 19, 2019

Uh oh!

GuillaumeFavelier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

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!

christian-oreilly commented Nov 22, 2019

Uh oh!

Uh oh!

agramfort commented Dec 8, 2019

Uh oh!

agramfort commented Dec 8, 2019

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!

christian-oreilly commented Dec 8, 2019

Uh oh!

agramfort commented Dec 8, 2019 via email

Uh oh!

christian-oreilly commented Dec 8, 2019

Uh oh!

christian-oreilly commented Dec 8, 2019

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 Dec 14, 2019

Uh oh!

Reviewers

Assignees

codecov bot commented Nov 21, 2019 •

edited

Loading