WIP: Set time_viewer and show_traces to auto

[GuillaumeFavelier]

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

It's an item of #7162

[GuillaumeFavelier]

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]

can you explain why?

[GuillaumeFavelier]

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]

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]

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

[GuillaumeFavelier]

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]

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]

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

[GuillaumeFavelier]

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

[GuillaumeFavelier]

Can we just do pytest<5.4.0 for now?

[GuillaumeFavelier]

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]

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

[GuillaumeFavelier]

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

[GuillaumeFavelier]

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

[larsoner]

Merged @GuillaumeFavelier

[GuillaumeFavelier]

#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]

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

[GuillaumeFavelier]

@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]

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

…

[GuillaumeFavelier]

This is ready for reviews then

[agramfort]

@larsoner merge if you're happy

[larsoner]

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]

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]

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]

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

I'll continue to investigate locally

[larsoner]

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]

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]

Thank you @larsoner

[larsoner]

LGTM +1 for merge from my end

@agramfort happy with this?

[agramfort]

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

[hoechenberger]

LGTM!