MRG: Link brains with _LinkViewer and plot_linked_brains #7227

GuillaumeFavelier · 2020-01-17T16:55:10Z

This PR adds the plot_linked_brains() function which contains a collection of _TimeViewer and share their properties. This is still a work in progress and only the time_slider is shared for now (we assume the same time array between the brains). This design provides independant apps and focus on what should be 'linked' with very little change. Please note that it can totally change depending on user needs/experience or performance.

Here is an example:

import os
import os
import mne
from mne.datasets import sample
from mne.viz import plot_linked_brains
data_path = sample.data_path()
sample_dir = os.path.join(data_path, 'MEG', 'sample')
subjects_dir = os.path.join(data_path, 'subjects')
fname_stc = os.path.join(sample_dir, 'sample_audvis-meg')
stc1 = mne.read_source_estimate(fname_stc, subject='sample')
stc2 = mne.read_source_estimate(fname_stc, subject='sample')
initial_time = 0.1
with mne.viz.use_3d_backend('pyvista'):
    brain1 = stc1.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                       clim=dict(kind='value', pos_lims=[3, 6, 9]),
                       time_viewer=True, hemi='split', views=['lat', 'med'])
    brain2 = stc2.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                       clim=dict(kind='value', pos_lims=[4, 6, 7]),
                       time_viewer=True)
    plot_linked_brains([brain1, brain2])

ToDo

Link playback_speed and play
Remember widget representation to avoid unnecessary loops: IntSlider, ShowView and SmartSlider (UpdateColorbarScale and BumpColorbarPoints will be done in another PR)
Rework naming convention to avoid confusion between class method and internal callback
Add plot_linked_brains to mne/viz/tests/test_3d.py
Add _LinkViewer to mne/viz/_brain/tests/test_brain.py
Remove the local patch for slider normalized coordinates in multi-view
Fix visibility issue of the sliders with toggle_interface
Fix inconsistency between UpdateColorbarScale and BumpColorbarPoints

Known issue

The slave brain slider handles are not udpated.

It's an item of #7162

hoechenberger · 2020-01-17T22:24:57Z

This is just one amazing unit of work you’re doing here!! ❤️❤️❤️

codecov · 2020-01-20T10:36:34Z

Codecov Report

Merging #7227 into master will increase coverage by <.01%.
The diff coverage is 96.45%.

@@            Coverage Diff             @@
##           master    #7227      +/-   ##
==========================================
+ Coverage   89.78%   89.79%   +<.01%     
==========================================
  Files         445      445              
  Lines       80241    80350     +109     
  Branches    12842    12857      +15     
==========================================
+ Hits        72043    72148     +105     
- Misses       5379     5381       +2     
- Partials     2819     2821       +2

GuillaumeFavelier · 2020-01-20T12:57:58Z

What can I use in the docstring of plot_linked_brains() as return type? It's complaining when I say the truth:

    """Plot multiple SourceEstimate objects with PyVista.

    Parameters
    ----------
    brains : list, tuple or np.ndarray
        The collection of brains to plot.

    Returns
    -------
    link_viewer : instance of _brain._LinkViewer
       The interactive interface to plot the linked brains.
    """

Or is it okay not to specify a return at all?

agramfort · 2020-01-20T13:00:17Z

only certain types of types are valid. Public functions should only link to public types. One solution is too keep _plot_linked_brain function private

…

larsoner · 2020-01-20T13:01:19Z

Could you just not return anything?

larsoner · 2020-01-20T13:02:04Z

(If there is a return you must describe it, but if you don't return anything then you don't need a returns section at all)

GuillaumeFavelier · 2020-01-20T13:02:47Z

I'll try returning nothing then if it does not work I'll make it private. Thank you.

GuillaumeFavelier · 2020-01-20T13:19:18Z

Now the play toggle and the playback_speed are linked too and no need to specify time_viewer=True in the example:

import os
import mne
from mne.datasets import sample
from mne.viz import plot_linked_brains
data_path = sample.data_path()
sample_dir = os.path.join(data_path, 'MEG', 'sample')
subjects_dir = os.path.join(data_path, 'subjects')
fname_stc = os.path.join(sample_dir, 'sample_audvis-meg')
stc1 = mne.read_source_estimate(fname_stc, subject='sample')
stc2 = mne.read_source_estimate(fname_stc, subject='sample')
initial_time = 0.1
with mne.viz.use_3d_backend('pyvista'):
    brain1 = stc1.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                       clim=dict(kind='value', pos_lims=[3, 6, 9]),
                       hemi='split', views=['lat', 'med'])
    brain2 = stc2.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                       clim=dict(kind='value', pos_lims=[4, 6, 7]))
    plot_linked_brains([brain1, brain2])

What else could be linked between the windows?

GuillaumeFavelier · 2020-01-22T20:48:22Z

This is caused by the outdated slider representation: It turns out that it is built at every update of the slider so it does not makes sense to store it. I thought it could help improve performance. I will revert.

GuillaumeFavelier · 2020-01-22T21:24:15Z

Not related at all to the representation: _TimeViewer wrapping was applied twice.

Let's look together tomorrow

Sure, my pleasure

larsoner · 2020-01-23T14:13:02Z

mne/viz/_brain/_timeviewer.py

            value=default_playback_speed,
-            rng=[0.01, 1], title="playback speed",
+            rng=[0.01, 1], title="play speed",


We should just use speed, it's short but still descriptive. Let's do it in another PR unless you're planning to push more commits

let's do it here. it's a very short change

larsoner

LGTM +1 for merge after:

latest.inc update
update to the capability table in the code, if relevant

* Add support for both in _TimeViewer * Port slider position patch from #7227

GuillaumeFavelier · 2020-01-27T09:44:30Z

I implemented @larsoner feedbacks and fix the conflicts with master.

This is ready to go on my end @agramfort

agramfort

also I still get the bug with the colormap change when pressing y. It switches to all red.

@GuillaumeFavelier @larsoner you want to do this in a follow up PR?

agramfort · 2020-01-27T19:51:20Z

mne/viz/_3d.py

+    brains : list, tuple or np.ndarray
+        The collection of brains to plot.
+    """
+    import collections


I would not nest this import

agramfort · 2020-01-27T19:51:54Z

mne/viz/_3d.py

+        The collection of brains to plot.
+    """
+    import collections
+    from ._brain import _Brain, _TimeViewer, _LinkViewer


do we need to do a check that this function requires pyvista backend?

agramfort · 2020-01-27T19:53:19Z

mne/viz/_3d.py

+            if not hasattr(brain, 'time_viewer') or brain.time_viewer is None:
+                brain = _TimeViewer(brain)
+        else:
+            raise TypeError("Expected type is _Brain but"


I would not refer to a private type.

Suggested change

raise TypeError("Expected type is _Brain but"

raise TypeError("Expected type is Brain but"

this is now a bit wrong but I prefer this.

agramfort · 2020-01-27T19:57:09Z

mne/viz/_brain/_timeviewer.py

+
+
+class SmartSlider(object):
+    """Class to manage smart slider."""


can you be more explicit. smart in what sense? What does it allow you to do?

GuillaumeFavelier · 2020-01-28T14:21:39Z

also I still get the bug with the colormap change when pressing y. It switches to all red.
you want to do this in a follow up PR?

+1 to do this in another PR

agramfort

Ok merge if CIs are green. Thx

larsoner · 2020-01-28T16:06:56Z

+1 to do this in another PR

Yes I still need to do the refactoring of _limits_to_control_points. Plenty of other stuff to work on in the meantime and I see this as a really minor use case issue -- it only happens if you are using auto-reset mode and you have one-sided data that you've chosen to display with a two-sided colormap, or vice-versa, so probably quite rare in real use cases.

larsoner · 2020-01-28T16:23:55Z

CircleCI is unrelated, thanks @GuillaumeFavelier !

* Add support for both in _TimeViewer * Port slider position patch from mne-tools#7227

) * Build first prototype * Fix docstring * Synchronize the slider handle * Add representation memory to IntSlider and ShowView * Introduce SmartSlider * Update time callback * Update orientation callback * Change default name of the time slider * Update smoothing call * Update fmin/fmid/fmax callbacks * Update fscale callback * Improve performance for auto_scaling * Use local _update_slider_callback * Improve plot_linked_brains docstring * Add plot_linked_brains to reference * Link play and playback speed * Update docstring * Add plot_linked_brains to tests * Add _LinkViewer to tests * Rename plot_linked_brains into link_brains * Remove local patch for normalized coordinate system * Fix double _TimeViewer wrapping * Fix issue with slider placement * Fix UI inconsistency * Change playback speed title to short version * Update latest and overview table * Fix name spelling * Implement reviews

* Add support for both in _TimeViewer * Port slider position patch from mne-tools#7227

) * Build first prototype * Fix docstring * Synchronize the slider handle * Add representation memory to IntSlider and ShowView * Introduce SmartSlider * Update time callback * Update orientation callback * Change default name of the time slider * Update smoothing call * Update fmin/fmid/fmax callbacks * Update fscale callback * Improve performance for auto_scaling * Use local _update_slider_callback * Improve plot_linked_brains docstring * Add plot_linked_brains to reference * Link play and playback speed * Update docstring * Add plot_linked_brains to tests * Add _LinkViewer to tests * Rename plot_linked_brains into link_brains * Remove local patch for normalized coordinate system * Fix double _TimeViewer wrapping * Fix issue with slider placement * Fix UI inconsistency * Change playback speed title to short version * Update latest and overview table * Fix name spelling * Implement reviews

Build first prototype

4000837

GuillaumeFavelier added the VIZ label Jan 17, 2020

GuillaumeFavelier self-assigned this Jan 17, 2020

GuillaumeFavelier mentioned this pull request Jan 17, 2020

ENH: New interface for Brain #7162

Closed

86 tasks

Fix docstring

e69cea5

Synchronize the slider handle

e9f2005

GuillaumeFavelier added 14 commits January 20, 2020 11:52

Add representation memory to IntSlider and ShowView

62e12a6

Introduce SmartSlider

4ffb901

Update time callback

e8ea40f

Update orientation callback

dc51a27

Change default name of the time slider

c817c17

Update smoothing call

e26c53a

Update fmin/fmid/fmax callbacks

0ca06ca

Update fscale callback

b7f2d51

Improve performance for auto_scaling

ffab732

Merge branch 'master' into time_viewer_plot_linked_brains

12b2e81

Use local _update_slider_callback

8b87a13

Improve plot_linked_brains docstring

e27016c

Add plot_linked_brains to reference

7ff6eb9

Link play and playback speed

7028bce

Update docstring

0a2399e

Add plot_linked_brains to tests

ec9cbc1

GuillaumeFavelier changed the title ~~MRG: Link brains with _LinkViewer and plot_linked_brains~~ WIP: Link brains with _LinkViewer and plot_linked_brains Jan 22, 2020

Fix double _TimeViewer wrapping

a5e068c

GuillaumeFavelier changed the title ~~WIP: Link brains with _LinkViewer and plot_linked_brains~~ MRG: Link brains with _LinkViewer and plot_linked_brains Jan 22, 2020

GuillaumeFavelier added 2 commits January 23, 2020 13:36

Fix issue with slider placement

bb304fc

Fix UI inconsistency

76e2e83

larsoner reviewed Jan 23, 2020

View reviewed changes

Change playback speed title to short version

361d273

larsoner approved these changes Jan 23, 2020

View reviewed changes

GuillaumeFavelier added 3 commits January 23, 2020 15:54

Update latest and overview table

ffbd9a7

Merge branch 'master' into time_viewer_plot_linked_brains

8a74730

Fix name spelling

d44e9c9

GuillaumeFavelier added a commit to GuillaumeFavelier/mne-python that referenced this pull request Jan 24, 2020

Port slider position patch from mne-tools#7227

498848d

GuillaumeFavelier mentioned this pull request Jan 24, 2020

FIX: Add support for hemi=both in _TimeViewer #7253

Merged

agramfort pushed a commit that referenced this pull request Jan 24, 2020

FIX: Add support for hemi=both in _TimeViewer (#7253)

0e66401

* Add support for both in _TimeViewer * Port slider position patch from #7227

Merge branch 'master' into time_viewer_plot_linked_brains

612ac90

agramfort reviewed Jan 27, 2020

View reviewed changes

Implement reviews

6d664d5

agramfort approved these changes Jan 28, 2020

View reviewed changes

larsoner merged commit f6e036d into mne-tools:master Jan 28, 2020

GuillaumeFavelier deleted the time_viewer_plot_linked_brains branch January 29, 2020 09:42

AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020

FIX: Add support for hemi=both in _TimeViewer (mne-tools#7253)

a3d49a4

* Add support for both in _TimeViewer * Port slider position patch from mne-tools#7227

AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020

FIX: Add support for hemi=both in _TimeViewer (mne-tools#7253)

f2bafcc

* Add support for both in _TimeViewer * Port slider position patch from mne-tools#7227

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG: Link brains with _LinkViewer and plot_linked_brains #7227

MRG: Link brains with _LinkViewer and plot_linked_brains #7227

GuillaumeFavelier commented Jan 17, 2020 •

edited

Loading

hoechenberger commented Jan 17, 2020

codecov bot commented Jan 20, 2020 •

edited

Loading

GuillaumeFavelier commented Jan 20, 2020

agramfort commented Jan 20, 2020 via email

larsoner commented Jan 20, 2020

larsoner commented Jan 20, 2020

GuillaumeFavelier commented Jan 20, 2020

GuillaumeFavelier commented Jan 20, 2020 •

edited

Loading

GuillaumeFavelier commented Jan 22, 2020

GuillaumeFavelier commented Jan 22, 2020 •

edited

Loading

larsoner Jan 23, 2020

GuillaumeFavelier Jan 23, 2020

larsoner left a comment

GuillaumeFavelier commented Jan 27, 2020

agramfort left a comment

agramfort Jan 27, 2020

agramfort Jan 27, 2020

agramfort Jan 27, 2020

agramfort Jan 27, 2020

GuillaumeFavelier commented Jan 28, 2020

agramfort left a comment

larsoner commented Jan 28, 2020

larsoner commented Jan 28, 2020

	raise TypeError("Expected type is _Brain but"
	raise TypeError("Expected type is Brain but"



		class SmartSlider(object):
		"""Class to manage smart slider."""

MRG: Link brains with _LinkViewer and plot_linked_brains #7227

MRG: Link brains with _LinkViewer and plot_linked_brains #7227

Conversation

GuillaumeFavelier commented Jan 17, 2020 • edited Loading

hoechenberger commented Jan 17, 2020

codecov bot commented Jan 20, 2020 • edited Loading

Codecov Report

GuillaumeFavelier commented Jan 20, 2020

agramfort commented Jan 20, 2020 via email

larsoner commented Jan 20, 2020

larsoner commented Jan 20, 2020

GuillaumeFavelier commented Jan 20, 2020

GuillaumeFavelier commented Jan 20, 2020 • edited Loading

GuillaumeFavelier commented Jan 22, 2020

GuillaumeFavelier commented Jan 22, 2020 • edited Loading

larsoner Jan 23, 2020

Choose a reason for hiding this comment

GuillaumeFavelier Jan 23, 2020

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

GuillaumeFavelier commented Jan 27, 2020

agramfort left a comment

Choose a reason for hiding this comment

agramfort Jan 27, 2020

Choose a reason for hiding this comment

agramfort Jan 27, 2020

Choose a reason for hiding this comment

agramfort Jan 27, 2020

Choose a reason for hiding this comment

agramfort Jan 27, 2020

Choose a reason for hiding this comment

GuillaumeFavelier commented Jan 28, 2020

agramfort left a comment

Choose a reason for hiding this comment

larsoner commented Jan 28, 2020

larsoner commented Jan 28, 2020

GuillaumeFavelier commented Jan 17, 2020 •

edited

Loading

codecov bot commented Jan 20, 2020 •

edited

Loading

GuillaumeFavelier commented Jan 20, 2020 •

edited

Loading

GuillaumeFavelier commented Jan 22, 2020 •

edited

Loading