Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Migrate VTK Widgets to GUI API #8862

Merged
merged 98 commits into from
Mar 1, 2021

Conversation

GuillaumeFavelier
Copy link
Contributor

This PR moves the VTK widgets into a dedicated Qt interface for the standalone solution and into Jupyter Widgets for notebooks.

Here is the latest sketch (it may change during development):

image

For the approach I chose, I'll just build the dock and move one widget at a time. So both old and new ways will live in parallel for a while. I expect working on refactoring and coverage at end. For now, it's a work in progress.

It's an item of #7162

@GuillaumeFavelier GuillaumeFavelier self-assigned this Feb 11, 2021
@GuillaumeFavelier
Copy link
Contributor Author

The previous interface relies on sliders heavily but I forgot that QSliders use integer by default and I don't seem to make the pyvistaqt.DoubleSlider work.

@larsoner
Copy link
Member

Don't use sliders in Qt, use QDoubleSpinBox es

@larsoner
Copy link
Member

(for time you probably do want a silder -- I would actually oversample the .times by a factor of 10 or so to give it a float-like behavior)

@GuillaumeFavelier
Copy link
Contributor Author

That's the easy way out of this for sure 😁

@GuillaumeFavelier
Copy link
Contributor Author

In the previous interface, each renderer had an orientation widget. On the sketch, there is only one widget for that. The simple solution is to duplicate those. A complicated one would be to command the click on a renderer to update the widget.

@larsoner
Copy link
Member

How about a SpinBox representing the raveled plotter number to control? Then at a later date we can connect some on_click action to update this SpinBox with whichever one was clicked, then you have interactive mouse-based control and direct indexing control.

@GuillaumeFavelier
Copy link
Contributor Author

How about a SpinBox representing the raveled plotter number to control?

This works too!

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 12, 2021

For now, this is how it looks:

image

The time controller is not a slider and the orientation widget is not migrated yet. Also, I find the clim/scale controller hard to use.

@GuillaumeFavelier
Copy link
Contributor Author

Now everything is migrated. I must admit, it is a cleaner 3D viewer:

image

The time widget is still not a slider tough.

@GuillaumeFavelier
Copy link
Contributor Author

Once the case of the time widget is closed, I think it will be a good time to double-check the UI/UX and fix the (hopefully) minor details. Then I'll move on to test/coverage. I'm wondering how LinkViewer will behave with this interface 🤔

And finally same treatment for the notebook backend.

@GuillaumeFavelier
Copy link
Contributor Author

I had modified the time callback back at the time because VTK sliders handle float values but QSlider support time index natively:

image

@GuillaumeFavelier
Copy link
Contributor Author

It's now ready for UI/UX reviews! Meanwhile I'll work on fixing the tests.

@larsoner
Copy link
Member

@GuillaumeFavelier sizing issue should be fixed, take a look at the code and _qt.py and _notebook.py etc. and make sure I didn't do something bad in terms of the hierarchy.

The sizing fix was QScrollArea + scroll.setMinimumSize(scroll.sizeHint().width, 0), i.e., make sure it always occupies its minimum width, then our _ensure_minimum_sizes and event processing takes care of the rest (i.e., resizing the window for us to accommodate the brain + dock). I also fixed the movie generation, no need to toggle the dock when capturing a movie anymore!

@agramfort
Copy link
Member

agramfort commented Feb 27, 2021 via email

@larsoner
Copy link
Member

screenshot now only takes a copy of the brain (no time series).

This behavior should change depending on whether you do time_viewer=True or (default) False

@agramfort
Copy link
Member

with time viewer True:
pouet2

with time viewer False:
pouet

using this code

import os
import mne
from mne.datasets import sample
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')
stc = mne.read_source_estimate(fname_stc, subject='sample')
initial_time = 0.13
time_viewer = False
# time_viewer = True
brain = stc.plot(subjects_dir=subjects_dir, initial_time=initial_time,
                 clim=dict(kind='value', lims=[3, 6, 9]),
                 hemi='split',
                 views=['lat', 'med'],
                 time_viewer=time_viewer
                 )

@GuillaumeFavelier
Copy link
Contributor Author

How do you take the screenshot @agramfort ?

@GuillaumeFavelier
Copy link
Contributor Author

I pushed a commit to fix:

AttributeError: module 'mne.viz.backends._qt' has no attribute '_close_3d_figure'
AttributeError: module 'mne.viz.backends._qt' has no attribute '_set_3d_view'

@GuillaumeFavelier
Copy link
Contributor Author

one you're living in "Qt land" and in the other you're living in "ipywidgets land"

I admit this separation is simpler 🏹 and I like _abstract.py too

@GuillaumeFavelier
Copy link
Contributor Author

Locally by using Brain in ipython, I obtain the size I request for the screenshot but during testing with pytest, test_brain_time_viewer still fails with:

===================================================================================================== FAILURES =====================================================================================================
_________________________________________________________________________________________ test_brain_time_viewer[pyvista] __________________________________________________________________________________________
mne/viz/_brain/tests/test_brain.py:506: in test_brain_time_viewer
    assert_allclose(img.shape, want_shape)
E   AssertionError: 
E   Not equal to tolerance rtol=1e-07, atol=0
E   
E   Mismatched elements: 1 / 3 (33.3%)
E   Max absolute difference: 282.
E   Max relative difference: 0.94
E    x: array([300, 582,   3])
E    y: array([300., 300.,   3.])

Reproduced in https://github.com/mne-tools/mne-python/runs/1989903394#step:13:3644 for example

@GuillaumeFavelier
Copy link
Contributor Author

I pushed a commit to fix the screenshot taken from the interface. I'm surprised it was not reported sooner.

@hoechenberger
Copy link
Member

I'm surprised it was not reported sooner.

Sorry 😢

@larsoner
Copy link
Member

larsoner commented Mar 1, 2021

Locally by using Brain in ipython, I obtain the size I request for the screenshot but during testing with pytest, test_brain_time_viewer still fails with:

This is because we toggle the interface off earlier in the test, which makes the 3D area take up the entire window width, which I'd consider expected behavior (though in the future we could consider shrinking the window). Toggling the interface back on before doing the screenshot fixes it locally, will push a fix.

@GuillaumeFavelier
Copy link
Contributor Author

I pushed a commit to fix AttributeError from _set_3d_title and _take_3d_screenshot as well.

@larsoner
Copy link
Member

larsoner commented Mar 1, 2021

Okay, let's get this in and iterate further on the interface as necessary. Thanks a ton @GuillaumeFavelier , this is great!

@larsoner larsoner merged commit 940a1fe into mne-tools:main Mar 1, 2021
@hoechenberger
Copy link
Member

@GuillaumeFavelier You're a wizard 🧙🏻‍♂️ Thank you!

larsoner added a commit to marsipu/mne-python that referenced this pull request Mar 1, 2021
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
  Fix indexing dipoles read from a bdip file (mne-tools#8963)
larsoner added a commit to adam2392/mne-python that referenced this pull request Mar 1, 2021
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
larsoner added a commit to bloyl/mne-python that referenced this pull request Mar 1, 2021
* upstream/main:
  MAINT: Skip matplotlib pre for now (mne-tools#8973)
  FIX: Brain lights (mne-tools#8972)
  MNT: Migrate VTK Widgets (mne-tools#8862)
  Fix (mne-tools#8971)
  Fix indexing dipoles read from a bdip file (mne-tools#8963)
@GuillaumeFavelier GuillaumeFavelier deleted the mnt/interface branch March 3, 2021 10:33
@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Mar 4, 2021
86 tasks
@GuillaumeFavelier GuillaumeFavelier changed the title MNT: Migrate VTK Widgets MNT: Migrate VTK Widgets to GUI API Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants