[ENH, MAINT] Refactor pyvistaqt/notebook widgets abstraction

[alexrockhill]

Fixes part of #10779, implemented in the hierarchical method described there, in short:

• The renderer has inherited from window implying its standalone application, to preserve this, we can still allow this although can also be a subunit within a GUI (e.g. coreg, brain timeviewer and ieeg_locate)
• We should allow windows to be instantiated without a renderer so that non-3D GUIs can use this framework as well and it works a lot better for inheritance (e.g. slice browser inheritance)

[alexrockhill]

Here's the progress on this:

The following works on a notebook:

import mne
mne.viz.set_3d_backend('notebook')
from mne.viz.backends.renderer import backend
central_layout = backend._VBoxLayout()
central_layout._add_widget(backend._Label('test'))
central_layout._add_widget(backend._Button('test2', callback=lambda: print('test3')))
window = backend._Window(central_layout=central_layout)
window._show()

and the following works with pyvistaqt:

import mne
mne.viz.set_3d_backend('pyvistaqt')
app = mne.viz.backends._utils._init_mne_qtapp()
from mne.viz.backends.renderer import backend
central_layout = backend._VBoxLayout()
from qtpy.QtWidgets import QLabel
central_layout._add_widget(backend._Label('test'))
button = backend._Button('test2', callback=lambda: print('test3'))
central_layout._add_widget(button)
window = backend._Window(central_layout=central_layout)
window.show()
app.exec_()

On the way, I've found several bugs, many things that aren't implemented and don't raise a not implemented error and lots of cruft e.g.

import os.path as op
import mne

data_path = mne.datasets.sample.data_path()
subjects_dir = op.join(data_path, 'subjects')
sample_dir = op.join(data_path, 'MEG', 'sample')

# gui = mne.gui.coregistration(subject='sample', subjects_dir=subjects_dir)

stc = mne.read_source_estimate(op.join(sample_dir, 'sample_audvis-meg'))
stc.crop(0.09, 0.1)
brain = mne.viz.Brain(subject_id='sample', subjects_dir=subjects_dir,
                      show_toolbar=True)
brain.add_data(stc.lh_data, hemi='lh', vertices=stc.lh_vertno)
brain.add_data(stc.rh_data, hemi='rh', vertices=stc.rh_vertno)

In [2]: brain.toggle_interface()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-10251cf1272f> in <module>
----> 1 brain.toggle_interface()

~/projects/mne-python/mne/viz/_brain/_brain.py in toggle_interface(self, value)
    585         """
    586         if value is None:
--> 587             self.visibility = not self.visibility
    588         else:
    589             self.visibility = value

AttributeError: 'Brain' object has no attribute 'visibility'

In [3]: brain.toggle_playback()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-bc92da9cfe2c> in <module>
----> 1 brain.toggle_playback()

~/projects/mne-python/mne/viz/_brain/_brain.py in toggle_playback(self, value)
    621         """
    622         if value is None:
--> 623             self.playback = not self.playback
    624         else:
    625             self.playback = value

AttributeError: 'Brain' object has no attribute 'playback'

Here's what I'm thinking; with a couple days of fixing, I think it could reimplement all the GUIs with this object-oriented programming but like @larsoner said, this has been established for maybe a year or more so we'd lose all the stability from that and I'm also at the point where I've learned a lot about metaclasses, both the backends and inheritance that this was a nice exercise whereas continuing on is just implementing it so it's a crossroads moment as far as either seeing it out and doing a hopefully not-too-painful rewrite of the widgets implementation or abandoning it and just picking our fights.

On one hand, I think the code written this way would 1) be less than half the lines of code 2) be much, much easier to add another backend, even though I don't see one coming, the mayavi transition makes me want MNE to be open to new tools and 3) be a lot easier to maintain in the long run because it is the inheritance scheme for both notebook and qt so everything is directly translated i.e. it just fits with how the backends work. On the other hand, it might break things for a while and there is no obvious soft transition to me at least (other than we could wait until after the 1.1 release and give it a full development cycle of testing once the merge button is pushed.

As far as the other option of abandoning, it's a lot of work for a couple days and I have other things to do so even though I've put in several hours thus far, I'm not opposed. I am really assured after looking into it that moving the ieeg_locate GUI to the backend is not the right move if the widgets are not refactored. I have plans for using inheritance that are totally incompatible from slice browser and in general, everything from the docks to the icons was designed for the brain viewer and not abstracted to a general-purpose GUI, especially one that doesn't have a 3D renderer. Thus, based on what I've seen, I have a strong, strong preference to close the PR to migrate the ieeg_locate GUI to the abstracted backend and table notebook compatibility (see #10819 for if it is even compatible anyway, very difficult to use or see anything and not sure how much time it would take to fix that) if the backend doesn't have this rewrite for the widgets.

I would like to hear input as this would be a PR that rewrote large portions of the backend and so should probably have someone reviewing which will be somewhat time-intensive. That being said, if I continue, I filmed the videos so that I would be comfortable with the function of the coreg GUI and I am comfortable with what the brain timeviewer is suppose to do so I wouldn't be before I was very sure that I could record those videos again. My preference is toward continuing on since I would like to see this cleaned up, I know it doesn't matter that much because it's not even user-facing, but I think these kinds of GUIs can do essential tasks that are not possible/very difficult with coding alone, and I don't think MNE is transitioning to an all-GUI interface any time soon but I could see there being a few more GUIs for data exploration and them being really helpful. cc @agramfort @larsoner @drammock @hoechenberger, if you had any thoughts that would be great.

[alexrockhill]

Oh I forgot a brief summary of what I did to get things to work:

• This is targeting the widgets, including _Window and it pulls the _Window stuff out of _Renderer
• All the abstract Widgets inherit from one _AbstractWidget, like how all the notebook ipywidgets inherit from Widget and all the qtpy.QtWidgets inherit from QWidget
  • This allows all the common methods to be shared through inheritance so that they aren't rewritten as they were previously over and over for dock and status bar and tool bar and menu (each of those had an add_xwidget method that was implemented independently and not standardized, this to me defeats the purpose of having ABCs at all whereas the new method uses them as intended)
• I plan to reimplement an abstract dock but all the menu and status and tool distinctions which weren't really meaningful anyway are getting replaced by _HBoxLayout, _VBoxLayout and _GridLayout. This is how it's done in both notebook and pyvistaqt so I'm just copying it with the idea that if both these development groups came up with this style of organization, we probably shouldn't reinvent the wheel and we should just rely on what they do. qtpy does have a QDockWidget but notebook doesn't have an equivalent to my knowledge and I haven't yet but I do plan on reimplementing one helper to set a bar either top, bottom, right or left with a specified thickness which takes care of all the bars.

In summary, because the style is changing, things might look a little bit different, other than for the ieeg_locate GUI which can be translated directly, basically with find and replace, but it will be very close and much easier to translate other GUIs that were made with notebook ipywidgets or in PyQt.

EDIT: One last thing that I thought would be nice but will be on a separate PR is to hide the main GUI when popups happen and then vice versa when you close so that you don't get this long output of windows stacking up. That would also fix the color picker for the ieeg_locate GUI which can't dropdown over the GUI in notebook but could just be the equivalent of a popup.

[larsoner]

@alexrockhill do I read it correctly that your preference is to refactor/rewrite the GUI abstraction code? If so, can you comment below #10779 (comment) with what the new structure would look like?

At the end of the day I'm okay with a big refactor as long as

1. All high-level Brain/coreg tests continue to pass as is (and low-level private-function-using gui tests pass using the new syntax
2. You are familiar enough with Brain/coreg to test locally to make sure all interactions of interest still work in the end
3. Things get less crufty :)

One way forward would be for you to put in a couple/few days of effort toward a refactoring then reassess. If it's 1-2 weeks of work in the end and much more maintainable/usable it's worth it, but if it's 1 month of work and things end up no more reliable than before, it's better we stop after a couple of days of trying.

[alexrockhill]

Great, complete agreement, I'll give it a couple days and see if I can get the GUIs working.

The structure is the same as for ipywidgets and Qt; there is a widget class and all the individual widgets inherit from it, ensuring consistent and DRY shared methods. There's really no more to it than that.

[alexrockhill]

Okay, major progress, took a couple days longer than expected, partly because a few things weren't working on main.

This code successfully executes all widgets used in coreg, mne.viz.Brain and ieeg_locate (except the color picker, in a followup PR, should be pretty quick though).

In pyvistaqt:

import mne
from qtpy.QtCore import QTimer
from matplotlib.backends.backend_qt5agg import FigureCanvas
from matplotlib.figure import Figure
mne.viz.set_3d_backend('pyvistaqt')
app = mne.viz.backends._utils._init_mne_qtapp()
from mne.viz.backends.renderer import backend
window = backend._Window()
central_layout = backend._VBoxLayout(scroll=(500, 500))
canvas = backend._MplCanvas(40, 40, 300)
canvas.ax.plot(range(10), range(10), label='plot')
central_layout._add_widget(canvas)
central_layout._add_widget(backend._Label('test'))
central_layout._add_widget(backend._Text('test', 'placeholder', lambda x: print(x)))
button = backend._Button('test2', callback=lambda: print('test3'))
central_layout._add_widget(button)
slider = backend._Slider(0, (0, 100), lambda x: print(x))
central_layout._add_widget(slider)
central_layout._add_widget(backend._CheckBox(0, lambda x: print(x)))
central_layout._add_widget(backend._SpinBox(10, (5, 50), lambda x: print(x), step=4))
central_layout._add_widget(backend._ComboBox('40', ('5', '50', '40'), lambda x: print(x)))
buttons = backend._RadioButtons('40', ('5', '50', '40'), lambda x: print(x))
central_layout._add_widget(buttons)
gb = backend._GroupBox('menu', [backend._Button(f'{i}', callback=lambda: print('test3')) for i in range(5)])
gb._set_enabled(False)
central_layout._add_widget(gb)
central_layout._add_widget(backend._FileButton(lambda name: print(name), window=window))
play_menu = backend._PlayMenu(0, (0, 100), lambda x: print(x))
central_layout._add_widget(play_menu)
pb = backend._ProgressBar(100)
timer = QTimer()
timer.timeout.connect(pb._update)
timer.setInterval(250)
timer.start()
central_layout._add_widget(pb)
window._add_keypress(lambda x: print(x))
'''
scroll_widget = backend._Widget()
hbox = backend._HBoxLayout()
hbox._add_widget(backend._ScrollArea(500, 500, scroll_widget))
scroll_widget._set_layout(central_layout)
'''
window._set_central_layout(central_layout)
window._set_focus()
window._show()
# pop up
#dialog = backend._Dialog('Info', 'this is a message', 'test', lambda x: print(x))

app.exec_()

In notebook:

%matplotlib widget
import mne
import matplotlib.pyplot as plt
mne.viz.set_3d_backend('notebook')
from ipywidgets import Output
import threading
from mne.viz.backends.renderer import backend
window = backend._Window()
central_layout = backend._VBoxLayout()
central_layout._add_widget(backend._Label('test'))
central_layout._add_widget(
    backend._Text('test', 'placeholder', lambda x: print(x)))
central_layout._add_widget(
    backend._Button('test2', callback=lambda: print('test3')))
slider = backend._Slider(0, (0, 100), lambda x: print(x))
central_layout._add_widget(slider)
central_layout._add_widget(
    backend._CheckBox(0, lambda x: print(x)))
central_layout._add_widget(
    backend._SpinBox(10, (5, 50), lambda x: print(x), step=4))
central_layout._add_widget(
    backend._ComboBox('40', ('5', '50', '40'), lambda x: print(x)))
central_layout._add_widget(
    backend._RadioButtons('40', ('5', '50', '40'), lambda x: print(x)))
gb = backend._GroupBox('menu', [backend._Button(
    f'{i}', callback=lambda: print('test3')) for i in range(5)])
gb._set_enabled(False)
central_layout._add_widget(gb)
central_layout._add_widget(
    backend._FileButton(lambda name: print(name), window=window))
# need out just for printing to terminal because of the play
# button to slider link
out = Output()
def play_callback(x):
    with out:
        print(x)
play_menu = backend._PlayMenu(0, (0, 100), play_callback)
central_layout._add_widget(play_menu)
central_layout._add_widget(out)
pb = backend._ProgressBar(100)

def update_pb():
    pb._update()
    threading.Timer(0.25, update_pb).start()
    
update_pb()
central_layout._add_widget(pb)
out = Output()
def key_callback(x):
    with out:
        print(x)
display(out)
window._add_keypress(key_callback)
# not part of widget, sent to the bottom because of matplotlib
# compatibility issues
canvas = backend._MplCanvas(5, 5, 96)
canvas.ax.plot(range(10), range(10), label='plot')
central_layout._add_widget(canvas)
window._set_central_layout(central_layout)
window._set_focus()
window._show()
dialog = backend._Dialog('Info', 'this is a message', 'test',
                         lambda x: print(x), window=window)

Since the code is exactly the same (minus a few outputs for the printlines which won't be in the widgets because they log to the console/mostly don't rely on printing), the GUI should be able to look fairly similar on both backends instead of having very different backend implementation (half of the notebook code wasn't implemented as well), having lots of redundancy and also looking pretty different widget-by-widget.

This should be the hardest part, now I just need to:

• Put the UI elements in mne.viz.Brain and coreg using this framework
• Make sure the tests pass
• Do a follow-up PR to refactor the ieeg_locate GUI to use this framework as well

A bit of a caveat that almost made me throw up my hands and give up on the notebook backend right at the last step is that it looks like there's some kind of workaround going on right now that wasn't in place in the past were matplotlib canvass are not treated like widgets but instead are just appended to the end. Since the only time plot is in mne.viz.Brain and it's at the bottom of the screen anyway, that's not too big of a deal. Although now that I'm writing this, I'm realizing that the slice plots in ieeg_locate and the future tfr_stc_viewer will not play well with this. Hmmm... Maybe the matplotlib devs will fix this, it was working in previous versions of matplotlib as in matplotlib/ipympl#203 and https://kapernikov.com/ipywidgets-with-matplotlib/ (output [24]). I tested those examples locally and it just throws them at the bottom of the output window in the wrong order. Might depend on a fix for this, I can open an issue.

[larsoner]

This code successfully executes all widgets

One option that might make this reasonable from a review standpoint would be:

1. PR 1: "just" make an abstraction layer and turn this into a test that works on both notebook and qt
2. PR 2: Transition coreg (and maybe brain, or maybe that's PR 2.5?) to use this new abstraction
3. PR 3: get iEEG-GUI to use this code

Might depend on a fix for this, I can open an issue.

If the code in that blog used to work but doesn't now, it seems like a MWE to post as a matplotlib issue. They are usually pretty quick to respond, and it seems like getting their canvas(es) to play nicely with ipywidget layouts would be core functionality.

[alexrockhill]

Sounds good, for 1 should I just use different file names for now? E.g. _notebook2

[alexrockhill]

Also it seems to work here so it could be I'm just missing a bit of magic https://matplotlib.org/ipympl/examples/full-example.html.

[larsoner]

Different filenames are fine, sure

[alexrockhill]

Here's how it looks currently

Scrolled down

Scrolled down more

Ok, I took a bit more time and cleaned up the sizes on notebook. Commit coming to try and integrate it into the GUIs, we can break it up if that doesn't work though,

[alexrockhill]

Ok, I got to here with reimplementing the mne.viz.Brain stc viewer GUI with the backend I made. It shouldn't be too much to finish it from there but I was mostly doing it to smooth out the backend which is was very helpful for doing so that I could use it in the GSoC tfr_stc_viewer project. I think for actually reimplementing this, I'd want to do it as a separate GUI script that just uses brain and pulls all the inline GUI stuff out of brain basically starting from scratch/this PR right now which I hope I will have time for later in the summer, I think that would be very nice for refactoring a lot of inelegance in Brain, I was already able to remove all the weakref stuff and it still worked, should be much cleaner soon.

I'm going to close this PR for now and open another one that just adds the backend to the existing _abstract.py, _qt.py and _notebook.py files and some tests so that I can use that abstraction for the new GSoC GUI and it will be pretty quick to use it for ieeg_locate as well, I just have to implement a color picker widget.

Per my conversation on Discord with @drammock and @hoechenberger, it would be nice if this backend was eventually used for coreg and mne.viz.Brain (although hopefully I'll have time for that this summer because it is pretty much in the scope of the GSoC) but we agreed per @drammock 's very apt suggestion that firm plans don't need to be made and we can live with the duplication for now until time permits.

I think the abstraction will be super useful for GSoC so that the TFR and indirectly the ieeg_locate GUIs will look really nice in notebook as well, I'm not a huge fan but it's definitely true that it is popular among users. Plus, the abstraction is actually really helpful for the Qt stuff as well, some of that can get a bit unwieldy and it's nice to have a simpler interface.

[larsoner]

Sounds good, if you convinced @drammock and @hoechenberger then I'm convinced by proxy :)

[alexrockhill]

One more point of housekeeping: @larsoner, what do you think about making the mne.viz.Brain methods show_time_viewer, plot_time_course and plot_time_line private? They haven't worked for me when not called through stc.plot so I think that would be more honest about how they are supported. I could mention that in the API changes section of latest to go through stc.plot when I do if you're in favor.

[larsoner]

Yes I think we should make more stuff in Brain private, especially the things that don't work