MRG: Enable point picking in _TimeViewer

[GuillaumeFavelier]

This PR enables point picking and display the vertex id as text in the top left. It's the first step to achieve #7235. Shortcut is p to pick under the mouse. This is still a work in progress.

Animation

ToDo

#7235 (comment)

Closes #7235
It's an item of #7162

[codecov]

Codecov Report

Merging #7247 into master will increase coverage by 0.02%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #7247      +/-   ##
==========================================
+ Coverage   89.93%   89.96%   +0.02%     
==========================================
  Files         451      451              
  Lines       81795    81846      +51     
  Branches    12977    12982       +5     
==========================================
+ Hits        73564    73630      +66     
+ Misses       5404     5385      -19     
- Partials     2827     2831       +4

[GuillaumeFavelier]

The shortcut is o to plot the timecourse of the latest picked point. For now it uses matplotlib for plotting.

[larsoner]

Mostly throwing out ideas here -- the most relevant VTK examples I could find are:

• https://lorensen.github.io/VTKExamples/site/Cxx/Plotting/LinePlot/
• https://lorensen.github.io/VTKExamples/site/Cxx/Plotting/ChartsOn3DScene/
• https://lorensen.github.io/VTKExamples/site/Cxx/Plotting/ScatterPlot/

We could put a time course up right over the 3D scene if you want (charts on 3D scene). But it might be cleaner to have the drawing area be split somehow, e.g. upper 2/3 for 3D lower 1/3 for 2D chart (if and only if show_traces=True, False by default) with the interaction being:

• click a point, the time course for that vertex is added to the chart, and a dot appears at that point on the surface
• click on a given the dot, that time course corresponding to that dot disappears from the chart and the point on the surface is removed

This would allow comparing multiple time courses very easily, which is nice. The dots that appear on the surface (probably from a scatter 3D sort of thing) could have the center colored according to the color that is used in the chart.

This would be a killer interactive data browsing feature for me personally...

(Someday, in the distant future, maybe we could also show time-frequency data by displaying a rectilinear grid on axes. But the time-course is much more important.)

[GuillaumeFavelier]

• click a point, the time course for that vertex is added to the chart, and a dot appears at that point on the surface
• click on a given the dot, that time course corresponding to that dot disappears from the chart and the point on the surface is removed

Wow, really nice UX. I will pin that in the issue as a todo list.

[GuillaumeFavelier]

Thank you so much @alexis-cvetkov for the help, the UX has been improved drastically!

[GuillaumeFavelier]

Thanks for the feedback @drammock, I'll make a pass to rename those

[GuillaumeFavelier]

This is the latest result:

[larsoner]

Awesome! Happy to try and push some fig.subplots_adjust / xlabel / ylabel etc. fixes if you want, or you can do it.

As far as picking goes I would say leave enable it by default if they have show_traces=True turned on, allowing them to hit p to turn it off

[GuillaumeFavelier]

Feel free to modify as you wish.

I have a question about show_traces though. Is it a parameter of link_brains() or plot_source_estimates ? Because _TimeViewer is not public.

[larsoner]

show_traces would be a parameter of plot_source_estimates

[larsoner]

BTW @GuillaumeFavelier like the volume viewer I think if show_traces=True then by default it should start with the vertex with the max abs value across time shown (as if it had already been clicked on). I can push this with my matplotlib tweaks if you want, but it might be better if you do it since you have a better idea of how to make this work with the _Brain internals.

[GuillaumeFavelier]

I'll take care of it 👍
I can't work on it right now but I will edit the todo list in the linked issue.

[larsoner]

Okay I'll test and push some mpl tweaks in the meantime, then

[larsoner]

The interaction is weird/not what I expected. Pressing "p" is when it picks a point. I think it should pick whenever you click. And clicking on the sphere that appears should make the point disappear.

[larsoner]

(Also a MPL figure window opens in addition to there being an embedded one, I'll try to fix this)

[GuillaumeFavelier]

P and R are not used at all for picking anymore, the left click with the mouse should be enough

okay so let's get rid of this? I think any unused options should go away

Sorry, maybe I didn't understand correctly. Get rid of what exactly? It shouldn't even be possible to use p or r at this state of the PR.

In general, I agree with you that we should focus on what's necessary 👍

[agramfort]

Linear temporal interpolation by default? It's what brain.save_movie will do in PySurfer. (Actually it uses quadratic interpolation, but this is probably overkill.) It makes viewing data smoother. But it's easy enough to use nearest instead by default if it's really problematic for people

Sorry for being slow here but why not indexing with integers the time axis and display the corresponding time in s or ms. I thought it was was we were doing. When I move along time axis I expect to move according to the sampling rate of the data. my 2c

[mmagnuski]

I agree with @agramfort - using the interface (dragging the time slider) I would expect to travel through time samples without any interpolation. However I understand it is important for movie playback. Is the movie playback bad without this time interpolation?

[jasmainak]

It shouldn't even be possible to use p or r at this state of the PR.

ummm ... I was getting a large text at the top of the scene saying I could use P or R. I think it appeared after I selected an option from the menu. Happy to take a look again once the PR is closer to being merged.

[larsoner]

Okay let's make nearest the default @GuillaumeFavelier . At some point we can make the interpolation mode easily publicly settable.

But I would suggest that people try the spacebar playback with linear mode and nearest mode so that you can fully judge the situation. People have seemed pleased about "smoothness" and if such comments had to do with the playback, nearest interpolation mode will be less smooth.

[larsoner]

... which you can do by setting brain.interp_kind = 'nearest' before starting playback (I think)

[hoechenberger]

Thinking about it a bit more, too -- any time-plot that we do in MNE that uses ax.plot instead of ax.step (and that's all of them AFAIK, including the time traces shown here) is doing linear interpolation in time rather than zero-order-hold / nearest interpolation in its depiction of the trace. So it's probably a more common practice than people realize

I think the main difference between the trace of a signal produced via ax.plot() and the activation displayed by _TimeViewer is that for the former, the user can typically see all values across all time points at once, while that's not the case for the source visualizations.

[GuillaumeFavelier]

Okay let's make nearest the default

It's noted on #7162 , I'll work on FakeVTKPicker now

I think it appeared after I selected an option from the menu.

I understand better now: the option for cell picking in the menu is not compatible with what we have when show_traces is enabled. It corresponds to PyVista default picking. I'll disable it to avoid the confusion.

[larsoner]

@GuillaumeFavelier I pushed a commit to more fully and cleanly parametrize one of the tests, and in adding 'rh' I found a bug:

mne/viz/_brain/tests/test_brain.py:166: in test_brain_timeviewer_traces
    time_viewer = _TimeViewer(brain_data, show_traces=True)
mne/viz/_brain/_timeviewer.py:283: in __init__
    self.plotter.subplot(ri, ci)
../pyvista/pyvista/plotting/plotting.py:272: in subplot
    raise IndexError('Column index is out of range ({})'.format(self.shape[1]))
E   IndexError: Column index is out of range (1)

[GuillaumeFavelier]

I encountered this one locally too, I will fix in this PR

[larsoner]

Thanks @GuillaumeFavelier !!!