WIP: Set time_viewer and show_traces to auto by GuillaumeFavelier · Pull Request #7380 · mne-tools/mne-python

GuillaumeFavelier · 2020-03-04T13:05:40Z

This PR sets time_viewer='auto' and show_traces='auto'. It's still a work in progress.

It's an item of #7162

GuillaumeFavelier · 2020-03-04T14:59:25Z

To achieve time_viewer=auto, the following 3d viz tests need to be ported to the renderer_interactive fixture:

test_plot_sparse_source_estimates
test_process_clim_plot

agramfort · 2020-03-04T15:03:32Z

can you explain why?

GuillaumeFavelier · 2020-03-04T15:06:50Z

By default the 3d tests are done with off_screen=True to avoid opening a window but in order to use the PyVista widgets, off_screen should be disabled (they require an interactive window).
The goal of the renderer_interactive fixture is just that.

codecov · 2020-03-11T09:09:23Z

Codecov Report

Merging #7380 into master will increase coverage by 0.01%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master    #7380      +/-   ##
==========================================
+ Coverage   90.09%   90.10%   +0.01%     
==========================================
  Files         453      453              
  Lines       82610    82622      +12     
  Branches    13062    13065       +3     
==========================================
+ Hits        74427    74447      +20     
+ Misses       5356     5351       -5     
+ Partials     2827     2824       -3

GuillaumeFavelier · 2020-03-11T09:13:04Z

The latest commit is marked as test because the renderer_interactive fixture does not include mayavi for now. It needs to be fixed.

GuillaumeFavelier · 2020-03-13T11:19:02Z

The interactive parameter of _testing_context() is unused in the _pysurfer_mayavi 3d backend:

mne-python/mne/viz/backends/_pysurfer_mayavi.py

Lines 464 to 472 in 499f917

    
           @contextmanager 
        
           def _testing_context(interactive): 
        
               mlab = _import_mlab() 
        
               orig_backend = mlab.options.backend 
        
               mlab.options.backend = 'test' 
        
               try: 
        
                   yield 
        
               finally: 
        
                   mlab.options.backend = orig_backend

compared to _pyvista:

mne-python/mne/viz/backends/_pyvista.py

Lines 647 to 661 in 499f917

    
           @contextmanager 
        
           def _testing_context(interactive): 
        
               from . import renderer 
        
               orig_offscreen = pyvista.OFF_SCREEN 
        
               orig_testing = renderer.MNE_3D_BACKEND_TESTING 
        
               if interactive: 
        
                   pyvista.OFF_SCREEN = False 
        
                   renderer.MNE_3D_BACKEND_TESTING = False 
        
               else: 
        
                   pyvista.OFF_SCREEN = True 
        
               try: 
        
                   yield 
        
               finally: 
        
                   pyvista.OFF_SCREEN = orig_offscreen 
        
                   renderer.MNE_3D_BACKEND_TESTING = orig_testing

So I think it's totally safe to add it to the list:

mne-python/mne/conftest.py

Lines 238 to 243 in 499f917

    
           @pytest.fixture(scope="module", params=[ 
        
               "pyvista", 
        
           ]) 
        
           def backend_name_interactive(request): 
        
               """Get the backend name.""" 
        
               yield request.param

GuillaumeFavelier · 2020-03-13T11:30:31Z

The tests using renderer_interactive are:

test_brain_timeviewer
test_brain_timeviewer_traces
test_brain_linkviewer
test_plot_sparse_source_estimates
test_process_clim_plot
test_link_brains

Among those, only:

test_plot_sparse_source_estimates
test_process_clim_plot

are supported by mayavi, the rest should expect NotImplementedError or pytest.skip()

GuillaumeFavelier · 2020-03-13T15:29:40Z

I forgot to update Circle config yesterday... The issue with pytest-sugar is back

GuillaumeFavelier · 2020-03-13T15:32:04Z

Oh no. It's actually another release of pytest:

GuillaumeFavelier · 2020-03-13T15:46:13Z

Can we just do pytest<5.4.0 for now?

GuillaumeFavelier · 2020-03-13T17:00:08Z

I notice a few warnings of the type:

UserWarning: Attempting to set identical left == right == 0 results in singular transformations; automatically expanding.

Also, there is an issue with time_label:

AttributeError: 'TimeSlider' object has no attribute 'time_label'

I'll push a fix.

larsoner · 2020-03-19T16:06:04Z

mne/viz/tests/test_3d.py

-                                        opacity=0.5, high_resolution=False)
-    if renderer.get_3d_backend() == 'mayavi':
-        import mayavi  # noqa: F401 analysis:ignore
-        assert isinstance(surf, mayavi.modules.surface.Surface)


Shouldn't we keep some variant of this? Can you instead change it to renderer_interactive.get_3d_backend()?

Indeed that was a bit too agressive, I'll add a branch for mayavi

GuillaumeFavelier · 2020-03-19T22:41:45Z

I'll try [circle full] once #7480 is merged

larsoner · 2020-03-19T23:05:03Z

Merged @GuillaumeFavelier

GuillaumeFavelier · 2020-03-20T08:00:41Z

#7480 was not enough apparently since we got a timeout too, I'm trying with show_traces=False and at the same time see if I can improve the memory footprint of _Brain

GuillaumeFavelier · 2020-03-20T08:44:20Z

So far my changes seem to help freeing memory but nothing big for _Brain (master in black, local changes in blue)

GuillaumeFavelier · 2020-03-20T09:06:27Z

@larsoner , @agramfort the timeout seems related to show_traces='auto' because now CircleCI passes:

https://18788-1301584-gh.circle-artifacts.com/0/dev/index.html

Is it good enough to have only time_viewer='auto'?

agramfort · 2020-03-20T09:09:46Z

yes ok. It can be improved later after the release.

…

GuillaumeFavelier · 2020-03-20T09:12:22Z

This is ready for reviews then

agramfort

@larsoner merge if you're happy

larsoner · 2020-03-20T13:08:05Z

Is it good enough to have only time_viewer='auto'?

yes ok. It can be improved later after the release.

I'm not so sure because this would be a change in behavior with no deprecation cycle.

A lesser evil would probably be to add a private _MNE_TRACES_AUTO=false env var that we set in CircleCI and detect in _brain to make it so 'auto' means False. This would make the output different from what a user would see when they ran the script with the PyVista backend and more like what it would be like with the Mayavi backend -- but I doubt anyone would really notice.

GuillaumeFavelier · 2020-03-20T13:18:26Z

A lesser evil would probably be to add a private _MNE_TRACES_AUTO=false env var

I guess it's possible indeed, what do you think @agramfort ?

Why private though @larsoner ?

larsoner · 2020-03-20T13:34:33Z

Why private though @larsoner ?

Hopefully eventually we can fix the memory/timeout problem and kill it the environment variable entirely. We don't want it actually to be a permanent solution (or give users yet another way to set things, they should use kwargs)

GuillaumeFavelier · 2020-03-20T14:17:08Z

Hopefully eventually we can fix the memory/timeout problem and kill it entirely

I'll continue to investigate locally

larsoner · 2020-03-20T15:11:28Z

Yes please do, but in the meantime the private env var workaround would make it so that we don't have to figure it out before release

larsoner · 2020-03-20T16:28:59Z

Pushed a commit with the CircleCI workaround and setting both show_traces and time_viewer to 'auto' as default, let's see if it gets us around the CI problem

GuillaumeFavelier · 2020-03-20T16:29:20Z

Thank you @larsoner

larsoner

LGTM +1 for merge from my end

@agramfort happy with this?

agramfort · 2020-03-23T21:26:38Z

if @drammock and @hoechenberger are happy I am happy. Happy @drammock and @hoechenberger ?

hoechenberger · 2020-03-23T21:30:35Z

LGTM!

TST: Set time_viewer and show_traces to auto

5040c58

GuillaumeFavelier added the VIZ label Mar 4, 2020

GuillaumeFavelier self-assigned this Mar 4, 2020

larsoner added this to the 0.20 milestone Mar 4, 2020

GuillaumeFavelier mentioned this pull request Mar 6, 2020

MNT: Revert to mayavi as 3d backend for building the documentation #7398

Closed

GuillaumeFavelier added 2 commits March 11, 2020 10:05

Merge branch 'master' into time_viewer_auto_parameter

9445d7f

TST: Port tests to renderer_interactive

499f917

This was referenced Mar 12, 2020

TST: Reproduce paintEvent failures #7294

Merged

ENH: New interface for Brain #7162

Closed

GuillaumeFavelier added 3 commits March 13, 2020 13:00

Update tests

726d540

Use renderer in test_mixed_sources_plot_surface

209374b

Merge branch 'master' into time_viewer_auto_parameter

ed05607

GuillaumeFavelier marked this pull request as ready for review March 13, 2020 12:22

TST: Trigger CIs [circle full]

7b8e085

GuillaumeFavelier mentioned this pull request Mar 13, 2020

MNT: Freeze pytest before 5.4.0 #7445

Closed

GuillaumeFavelier added 2 commits March 13, 2020 17:14

Merge branch 'master' into time_viewer_auto_parameter

f79e9f3

TST: Trigger CIs [circle full]

cf347a3

GuillaumeFavelier added 2 commits March 13, 2020 18:16

Fix minor issues

13b3ba8

TST: Trigger CIs [circle full]

bd82eaf

larsoner reviewed Mar 19, 2020

View reviewed changes

GuillaumeFavelier added 2 commits March 19, 2020 23:37

Merge branch 'master' into time_viewer_auto_parameter

77607f4

Bring back the surface test

15c3a6d

GuillaumeFavelier added 3 commits March 20, 2020 00:10

Merge branch 'master' into time_viewer_auto_parameter

edc10a8

TST: Trigger CIs [circle full]

312c4d5

TST: Try with time_viewer=True and show_traces=False [circle full]

26cb47e

GuillaumeFavelier changed the title ~~WIP: Set time_viewer and show_traces to auto~~ MRG: Set time_viewer and show_traces to auto Mar 20, 2020

agramfort approved these changes Mar 20, 2020

View reviewed changes

GuillaumeFavelier changed the title ~~MRG: Set time_viewer and show_traces to auto~~ WIP: Set time_viewer and show_traces to auto Mar 20, 2020

FIX: Both auto [circle full]

1ceb660

larsoner approved these changes Mar 23, 2020

View reviewed changes

drammock merged commit b884155 into mne-tools:master Mar 23, 2020

GuillaumeFavelier mentioned this pull request Mar 25, 2020

WIP: Use _Brain in plot_vector_source_estimates #7060

Merged

17 tasks

GuillaumeFavelier deleted the time_viewer_auto_parameter branch June 11, 2020 09:50

Uh oh!

Conversation

GuillaumeFavelier commented Mar 4, 2020

Uh oh!

GuillaumeFavelier commented Mar 4, 2020

Uh oh!

agramfort commented Mar 4, 2020 via email

Uh oh!

GuillaumeFavelier commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GuillaumeFavelier commented Mar 11, 2020

Uh oh!

GuillaumeFavelier commented Mar 13, 2020

Uh oh!

GuillaumeFavelier commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeFavelier commented Mar 13, 2020

Uh oh!

GuillaumeFavelier commented Mar 13, 2020

Uh oh!

GuillaumeFavelier commented Mar 13, 2020

Uh oh!

GuillaumeFavelier commented Mar 13, 2020

Uh oh!

larsoner Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

GuillaumeFavelier Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

GuillaumeFavelier commented Mar 19, 2020

Uh oh!

larsoner commented Mar 19, 2020

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

GuillaumeFavelier commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Mar 20, 2020 via email

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Mar 20, 2020

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

larsoner commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

larsoner commented Mar 20, 2020

Uh oh!

larsoner commented Mar 20, 2020

Uh oh!

GuillaumeFavelier commented Mar 20, 2020

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort commented Mar 23, 2020

Uh oh!

hoechenberger commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

GuillaumeFavelier commented Mar 4, 2020 •

edited

Loading

codecov bot commented Mar 11, 2020 •

edited

Loading

GuillaumeFavelier commented Mar 13, 2020 •

edited

Loading

GuillaumeFavelier commented Mar 20, 2020 •

edited

Loading

larsoner commented Mar 20, 2020 •

edited

Loading