ENH: new coreg gui features #10015

GuillaumeFavelier · 2021-11-17T16:13:12Z

This PR adds support for the parameters scale_by_distance, project_eeg, mark_inside of CoregistrationUI.

It's part of #8833

ToDo

integrate head_transparency, scale_by_distance, mark_inside and orient_to_surface within the UX
show/hide HPI/HSP/EEG based on the weight they are assigned for ICP
fix bug with head_resolution widget not being sync'ed with fid_accurate
add a status bar to display infos about fitting operations
improve 3d labels content
fix bug with ~~mark_inside on maOS~~ pyvista<0.32

agramfort · 2021-11-17T20:26:14Z

tell us when it's ready for review @GuillaumeFavelier

GuillaumeFavelier · 2021-11-18T14:41:51Z

I double-checked some examples and fixed a visual bug introduced in here. This PR is now ready for reviews @agramfort, @larsoner

larsoner · 2021-11-18T16:03:51Z

I ran:

mne coreg -s sample -d subjects --fif MEG/sample/sample_audvis_raw.fif

clicked "lock" (to get out of fiducials mode), then hit "fit fiducials", then turned on transparent mode, then recorded this, which is just a few clusters of rapid Z translation clicks:

Peek.2021-11-18.11-02.mp4

After I stop clicking the Z-translate button, the points jump downward (probably correct?) but then "slide upward" incrementally (probably incorrect?). Note that this does not happen to HPI coils but it does to EEG and dig points, if that helps track down the bug.
The HPI-coil-size is seems wrong for the coil on the far side of the head. By the end of the video, it's basically on the head surface, but it has a huge radius

mne/gui/_coreg.py

GuillaumeFavelier · 2021-11-22T14:37:43Z

just a few clusters of rapid Z translation clicks

I added a lock on _set_parameter() which should prevent those "out-of-sync" scenarii caused by spamming.

GuillaumeFavelier

https://github.com/GuillaumeFavelier/mne-python/blob/cc281f88fff60bc7bdd522e9c369421a413fc81c/mne/gui/_coreg.py#L221

Is this really necessary? Because it is not compatible with the parameter head_resolution (for example when the user sets --low-res-head).

GuillaumeFavelier · 2021-11-22T15:18:21Z

In an attempt to simplify interface and user interactions, what do you think of:

merging orient_glyphs and scale_by_distance into one checkbox
keeping mark_inside=True and removing its checkbox
setting head_transparency depending on "lock fiducials" and removing its checkbox

Also is it necessary to have all "Show HPI", "Show EEG" and "Show HSP", maybe only one "Show sensors" could work?
If those widgets are not really needed, we can remove them all too.

larsoner · 2021-11-22T17:17:32Z

merging orient_glyphs and scale_by_distance into one checkbox

From talking to @agramfort a bit, I'm tempted to just always scale by distance and orient the glyphs, and not make these optional.

keeping mark_inside=True and removing its checkbox

Agreed

setting head_transparency depending on "lock fiducials" and removing its checkbox

Sometimes it's nice to be able to make it transparent, and even to varying degrees. But I guess we could add support for this later, and stick with what you propose for now

Also is it necessary to have all "Show HPI", "Show EEG" and "Show HSP", maybe only one "Show sensors" could work?

Being able to toggle these separately can be nice. What if we show/hide based on the weight they are assigned for ICP? Zero weight = hide, non-zero weight = show?

larsoner · 2021-11-22T17:20:05Z

Is this really necessary? Because it is not compatible with the parameter head_resolution (for example when the user sets --low-res-head).

You need to use the high resolution head when picking fiducials, otherwise your clicks get locked to vertex locations that are some mm away from where you actually clicked. Also it's much harder to make out the correct fid locations when using the low resolution head.

In other words, I think if you are in "set fiducials" mode, you should always be shown an opaque, high-res head with no sensors. Once you leave that mode, you should switch to a semi-transparent, high- or low- (depending on the option / config value they have set) res head with whatever sensors they've chosen to show

larsoner · 2021-12-03T13:46:23Z

Sure, I can look today or early next week

larsoner · 2021-12-03T17:11:41Z

Fixed the drawing (I think)
Last update always gets drawn by the timer. You can verify interactively, but also by watching e.g.:
```
MNE_LOGGING_LEVEL=debug mne coreg -s sample -d subjects/ --fif MEG/sample/sample_audvis_raw.fif
```
Fixed "jerkiness" by removing a _process_events call in _add_mesh. We really shouldn't need it there. If we do, we should fix it elsewhere. I think this should speed things up globally, because now we can truly manually control when updates are made. @GuillaumeFavelier if you remember where this came from, we should double check that removing it doesn't break anything!
Fixed CTRL-C behavior (now properly closes rather than aborting) both for mne coreg and for PyQtGraph-based browsing
Fixed iterative ICP updates so they show up on the fly now. I can't upload a video at the moment, but feel free to try locally!

larsoner · 2021-12-03T17:12:59Z

mne/viz/backends/_utils.py

+# https://stackoverflow.com/questions/5160577/ctrl-c-doesnt-work-with-pyqt
+def _qt_app_exec(app):
+    # adapted from matplotlib
+    old_signal = signal.getsignal(signal.SIGINT)
+    is_python_signal_handler = old_signal is not None
+    if is_python_signal_handler:
+        signal.signal(signal.SIGINT, signal.SIG_DFL)
+    try:
+        app.exec_()
+    finally:
+        # reset the SIGINT exception handler
+        if is_python_signal_handler:
+            signal.signal(signal.SIGINT, old_signal)


cc @marsipu you'll probably be interested in this -- it makes ctrl-C actually interrupt blocking mode. For me it actually kills the Python session, but matplotlib does the same. So I think it's okay (?)

larsoner · 2021-12-03T17:13:30Z

mne/viz/utils.py

        if show:
            fig.show()
        # If block=False, a Qt-Event-Loop has to be started
        # somewhere else in the calling code.
        if block:
-            QApplication.instance().exec()
+            _qt_app_exec(QApplication.instance())


... and here is where I used that private function it so that it would affect pyqtgraph-based browsing

larsoner · 2021-12-03T20:57:19Z

Pushed another couple of commits to:

Temporarily use my PyVista version branch until they change their policy (FIX: Bump version pyvista/pyvista#1911)
Make hpi the same size as dig and EEG
Default to "matched" fid mode

@GuillaumeFavelier can you look and see if my changes and the code behavior make sense? Basically every _update_plots call just updates the set of things to draw, and every tick of the time (1/60th of a sec max) _redraw is called, which pops things from the list of things to draw. This ensures that whatever our last state is, eventually all items from it get drawn. In practice it seems that they're always drawn at once, which is nice. (Before they were drawn at the same time, but the _process_events in _add_mesh caused the draw to update, so if you updated say 5 items it would evoke 5 redraws rather than a single one at the end.)

GuillaumeFavelier · 2021-12-06T11:23:19Z

Thank you for the great help @larsoner!

can you look and see if my changes and the code behavior make sense?

I took the time to review your commits carefully and I tested locally with _qt and _notebook. I'm glad that 2 mutexes and a set did the trick 👌

Fixed CTRL-C behavior

This is awesome!

Fixed iterative ICP updates so they show up on the fly now

I was not aware of this issue, for me, the plot was updated after each iteration 😮

Fixed "jerkiness" by removing a _process_events call in _add_mesh.

Same for the jerkiness but it was maybe more difficult for me to notice.

Before they were drawn at the same time, but the _process_events in _add_mesh caused the draw to update, so if you updated say 5 items it would evoke 5 redraws rather than a single one at the end.

Anyway, thank you for the explanation, I'll keep this in mind.

We really shouldn't need it there.

I have been digging to find where the call comes from and I tracked it down to #7791 (comment). Apparently, it was necessary during the migration to pyvistaqt on MacOS with vtk < 8.2.

To be honest, we probably do not need it anymore.

agramfort · 2021-12-06T12:00:24Z

trying to unselect "show high resolution head" on my mac produces a segfault. Can someelse replicate? Just untick the box just after starting the GUI.

❯ mne coreg -s sample -d ~/mne_data/MNE-sample-data/subjects --interaction=trackball -f ~/mne_data/MNE-sample-data/MEG/sample/sample_audvis_raw.fif
python3.8(72101,0x10aa1cdc0) malloc: Incorrect checksum for freed object 0x7fef1ba26a00: probably modified after being freed.
Corrupt value: 0x101010101010101
python3.8(72101,0x10aa1cdc0) malloc: *** set a breakpoint in malloc_error_break to debug
Fatal Python error: Aborted

Current thread 0x000000010aa1cdc0 (most recent call first):
File "/Users/alex/miniconda3/lib/python3.8/site-packages/vtkmodules/qt/QVTKRenderWindowInteractor.py", line 381 in paintEvent
File "/Users/alex/work/src/mne-python/mne/viz/backends/_utils.py", line 163 in _qt_app_exec
File "/Users/alex/work/src/mne-python/mne/gui/_coreg.py", line 249 in init
File "", line 24 in init
File "/Users/alex/work/src/mne-python/mne/gui/init.py", line 192 in coregistration
File "", line 24 in coregistration
File "/Users/alex/work/src/mne-python/mne/commands/mne_coreg.py", line 89 in run
File "/Users/alex/work/src/mne-python/mne/commands/utils.py", line 107 in main
File "/Users/alex/miniconda3/bin/mne", line 33 in
[1] 72101 abort mne coreg -s sample -d ~/mne_data/MNE-sample-data/subjects -f

GuillaumeFavelier · 2021-12-06T12:20:25Z

trying to unselect "show high resolution head" on my mac produces a segfault.

I have the same behaviour

agramfort · 2021-12-06T12:23:50Z

you're on it?

…

GuillaumeFavelier · 2021-12-06T12:35:30Z

yes, trying to see what is going on suddenly with this head :)

GuillaumeFavelier · 2021-12-06T14:27:41Z

The crash seems related to the _grow_hair_changed() call in _hair_resolution_changed() so I pushed a few commits to rework grow_hair and obtain something compatible with the new redraw system. What do you think @larsoner, @agramfort ?

larsoner · 2021-12-06T16:07:06Z

I pushed a few commits to rework grow_hair and obtain something compatible with the new redraw system. What do you think @larsoner, @agramfort ?

When I saw your comment about it being related to the hair growth stuff, my thought was "we should separate the update of the head geometry variable from the update of the PyVista plot", and it looks like indeed that's what you did, so I'll go ahead and fix the conflict and merge hopefully. I also noticed in @hoechenberger's PR some pyvista version comparisons we no longer need I think.

* upstream/main: Use fixes._compare_version for version checks everywhere (mne-tools#10091) Fast annotation from mask (mne-tools#10089) fix trace offsets in butterfly mode (mne-tools#10087) fix plot_compare_evokeds topo legend axes placement (mne-tools#9927) doc: clarify ica.apply include and exclude params (mne-tools#10086) MRG: Make y a required parameter in CSP.fit_transform() (mne-tools#10084) Add scrollbar to report tag dropdown menu (mne-tools#10082)

larsoner · 2021-12-06T18:28:29Z

Thanks @GuillaumeFavelier !

GuillaumeFavelier added 4 commits November 17, 2021 13:03

add support for project_eeg and scale_by_distance

42c0ec1

add support for mark_inside

6e18a4d

add doc

76302d1

fix

d6f6a5f

GuillaumeFavelier self-assigned this Nov 17, 2021

GuillaumeFavelier changed the title ~~WIP,ENH: new coreg features~~ WIP,ENH: new coreg gui features Nov 17, 2021

fix

f256f53

GuillaumeFavelier changed the title ~~WIP,ENH: new coreg gui features~~ ENH: new coreg gui features Nov 18, 2021

larsoner reviewed Nov 18, 2021

View reviewed changes

mne/gui/_coreg.py Outdated Show resolved Hide resolved

change default

7e75f3e

GuillaumeFavelier changed the title ~~ENH: new coreg gui features~~ WIP,ENH: new coreg gui features Nov 22, 2021

sync _set_parameter with a lock

cc281f8

GuillaumeFavelier commented Nov 22, 2021

View reviewed changes

GuillaumeFavelier added 2 commits November 22, 2021 16:02

Merge branch 'main' into enh/coreg_feats

6bd5da8

rename

c9e91d2

GuillaumeFavelier added 8 commits November 23, 2021 14:49

simplify view options

ca0d66f

simplify view options

46413ff

fix

bb0a7dc

Merge branch 'main' into enh/coreg_feats

794a772

fix

54e435f

simplify view options

9e3c434

improve fit_icp log

513dd4e

refactor

1ca1191

GuillaumeFavelier mentioned this pull request Dec 3, 2021

ENH: new coreg gui features (part 2) #10085

Merged

larsoner added 2 commits December 3, 2021 08:30

Merge remote-tracking branch 'upstream/main' into enh/coreg_feats

895a2c1

ENH: Fixes

a5115ca

larsoner reviewed Dec 3, 2021

View reviewed changes

larsoner added 4 commits December 3, 2021 09:15

FIX: Cruft

0bb167e

FIX: Cruft

6a631e2

BUG: Work around PyVista version weirdness

8a534f8

FIX: Two mare

8f21628

FIX: Test

534d845

larsoner mentioned this pull request Dec 3, 2021

Enable fNIRS in coreg gui #10081

Closed

GuillaumeFavelier changed the title ~~WIP,ENH: new coreg gui features~~ ENH: new coreg gui features Dec 6, 2021

GuillaumeFavelier added 2 commits December 6, 2021 14:49

fix hair

0e73e5d

refactor

d77b88d

larsoner added 3 commits December 6, 2021 11:08

MAINT: Remove some comparisons

a8ac18b

FIX: Restore link

64e1dbc

larsoner merged commit 0ad069b into mne-tools:main Dec 6, 2021

GuillaumeFavelier deleted the enh/coreg_feats branch December 6, 2021 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: new coreg gui features #10015

ENH: new coreg gui features #10015

GuillaumeFavelier commented Nov 17, 2021 •

edited

Loading

agramfort commented Nov 17, 2021

GuillaumeFavelier commented Nov 18, 2021

larsoner commented Nov 18, 2021

GuillaumeFavelier commented Nov 22, 2021

GuillaumeFavelier left a comment •

edited

Loading

GuillaumeFavelier commented Nov 22, 2021

larsoner commented Nov 22, 2021

larsoner commented Nov 22, 2021 •

edited

Loading

larsoner commented Dec 3, 2021

larsoner commented Dec 3, 2021 •

edited

Loading

larsoner Dec 3, 2021

larsoner Dec 3, 2021

larsoner commented Dec 3, 2021

GuillaumeFavelier commented Dec 6, 2021

agramfort commented Dec 6, 2021

GuillaumeFavelier commented Dec 6, 2021

agramfort commented Dec 6, 2021 via email

GuillaumeFavelier commented Dec 6, 2021

GuillaumeFavelier commented Dec 6, 2021

larsoner commented Dec 6, 2021

larsoner commented Dec 6, 2021

ENH: new coreg gui features #10015

ENH: new coreg gui features #10015

Conversation

GuillaumeFavelier commented Nov 17, 2021 • edited Loading

agramfort commented Nov 17, 2021

GuillaumeFavelier commented Nov 18, 2021

larsoner commented Nov 18, 2021

GuillaumeFavelier commented Nov 22, 2021

GuillaumeFavelier left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeFavelier commented Nov 22, 2021

larsoner commented Nov 22, 2021

larsoner commented Nov 22, 2021 • edited Loading

larsoner commented Dec 3, 2021

larsoner commented Dec 3, 2021 • edited Loading

larsoner Dec 3, 2021

Choose a reason for hiding this comment

larsoner Dec 3, 2021

Choose a reason for hiding this comment

larsoner commented Dec 3, 2021

GuillaumeFavelier commented Dec 6, 2021

agramfort commented Dec 6, 2021

GuillaumeFavelier commented Dec 6, 2021

agramfort commented Dec 6, 2021 via email

GuillaumeFavelier commented Dec 6, 2021

GuillaumeFavelier commented Dec 6, 2021

larsoner commented Dec 6, 2021

larsoner commented Dec 6, 2021

GuillaumeFavelier commented Nov 17, 2021 •

edited

Loading

GuillaumeFavelier left a comment •

edited

Loading

larsoner commented Nov 22, 2021 •

edited

Loading

larsoner commented Dec 3, 2021 •

edited

Loading