mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

ENH: New interface for Coregistration

Open GuillaumeFavelier opened this issue 4 years ago • 15 comments

This PR summarizes the ideas to implement for coreg:

Milestone 1.0
  • [x] increase fiducial picking performance (https://github.com/mne-tools/mne-python/pull/9689#issuecomment-951910957)
  • [x] add support for the project_eeg parameter
  • [x] add support for the scale_by_distance parameter
  • [x] add support for the mark_inside parameter
  • [x] console/status_bar idea to add to the UI whatever is printed in the terminal
  • [x] display dist estimation in mm.
  • [x] add units to UI (Omit more explicit message)
  • [x] display axes
  • [x] add tooltips as much as possible on widgets
  • [x] add support for the scaling parameters (https://github.com/mne-tools/mne-python/pull/10117)

Bonus territory:

  • [x] checkbox for the MEG helmet
  • [x] add a slider to control transparency
  • [x] add a button to save the fiducials (https://github.com/mne-tools/mne-python/issues/10208#issuecomment-1016402749)
  • [x] rename "Transform" groupbox into "HEAD <> MRI Transform"
  • [x] find a better name for the "Digitization Source" section (which could be confusing)
  • [x] show a dialog to save if trans is not saved (https://github.com/mne-tools/mne-python/issues/10236)
  • [x] add a prompt check name already exists "overwrite or not"
Milestone 1.1
  • [x] add legend to plot alignment / coreg to see what colored points are etc.
  • [x] add support for (qdarkstyle) dark theme
  • [x] 📁 icon for file button widgets (instead of "Load")
  • [ ] multiple threads should block window exit
  • [ ] add support for object_fit='contain' and object_position='top' (ipywidgets) (https://github.com/Kitware/ipyvtklink/issues/35, https://github.com/mwcraig/ipyevents/issues/71)
Done
  • [ ] ~~add support for the guess_mri_subject parameter~~ (YAGNI)
  • [ ] ~~add support for the head_inside parameter~~ (YAGNI)
  • [x] Update Coregistration() to require parameters (https://mne.discourse.group/t/mne-coreg-based-on-ctf-data/3022/4)
  • [x] make the sections collapsible (ipywidgets)
  • [x] Decouple GUI code from model (suggested in https://github.com/mne-tools/mne-python/pull/6693#issuecomment-542364434, done in https://github.com/mne-tools/mne-python/pull/9516)
  • [x] Build a coreg GUI using pyvista (done in https://github.com/mne-tools/mne-python/pull/9689)
Bugs
  • [x] the parameter spin boxes force 2 updates consecutively even though only one interaction is done
  • [x] multi-line labels are not supported (ipywidgets)
  • [x] Qt crashes during _configure_status_bar() (Windows)
  • [x] the app is abnormally slow at loading FIFF (Windows)
  • [x] inconsistent behaviour of checkbox layout (ipywidgets)
  • [x] the status bar does not support dark theme (qt) (https://github.com/mne-tools/mne-python/pull/10238#issuecomment-1079155443)
  • [ ] the spin boxes are not updated during ICP with scaling (https://github.com/mne-tools/mne-python/pull/10377#issuecomment-1061901037)
  • [ ] the DigPoints are displayed with unlocked fiducials when switching subjects (https://github.com/mne-tools/mne-python/pull/10242#issuecomment-1023977033)
  • [ ] all HSPs become marked as inside the head surface on certain scaling values (https://github.com/mne-tools/mne-python/pull/10224#issuecomment-1017592043)
  • [ ] the app hangs when loading trans (ipywidgets)

GuillaumeFavelier avatar Feb 03 '21 09:02 GuillaumeFavelier

@GuillaumeFavelier as the next step, can you separate into blockers for 0.24 and non-blockers for 0.24, perhaps working with @agramfort and @hoechenberger ? To me it's just the initial-startup bug I added to the list.

larsoner avatar Oct 27 '21 19:10 larsoner

Alright, I'll update the original post and take care of this bug in priority 👍

GuillaumeFavelier avatar Oct 27 '21 19:10 GuillaumeFavelier

@GuillaumeFavelier - thanks so much for this refactoring. I'm really looking forward to leaving behind mayavi for coreg.

In updating some of my code, i ran into an issue with the auto coreg method and CTF data. CTF doesn't have unique HPIs (they use nas/lpa/rpa) and the current code assumes these are not None and tries to update them.

Below is an MWE that throws

~/anaconda3/envs/mne-stable/lib/python3.9/site-packages/mne/coreg.py in _update_params(self, rot, tra, sca, force_update_omitted)
 -> 1464                 apply_trans(self._head_mri_t, self._dig_dict['hpi'])
 
~/anaconda3/envs/mne-stable/lib/python3.9/site-packages/mne/transforms.py in apply_trans(trans, pts, move)
 -> 240     out_pts = np.dot(pts, trans[:3, :3].T)
 
TypeError: unsupported operand type(s) for *: 'NoneType' and 'float'
import os.path as op
import numpy as np

import mne
from mne.coreg import Coregistration
from mne.io import read_info, read_raw_ctf


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

ctf_dir = op.join(mne.datasets.testing.data_path(download=False), 'CTF')
ctf_fname_catch = op.join(ctf_dir, 'catch-alp-good-f.ds')
raw = read_raw_ctf(ctf_fname_catch)
fiducials = "estimated"  # get fiducials from fsaverage
coreg = Coregistration(raw.info, subject, subjects_dir, fiducials=fiducials)

This patch moves past the error allowing CLI coregistration, but I don't know how it interacts with the work you're doing in the GUI or if there is another way you would want to solve the issue so didn't want to open an separate PR.

diff --git a/mne/coreg.py b/mne/coreg.py
index dba41f8ec..d09ff7287 100644
--- a/mne/coreg.py
+++ b/mne/coreg.py
@@ -1373,6 +1373,9 @@ class Coregistration(object):
             self._dig_dict['nasion'] = \
                 np.array([self._dig_dict['nasion']], float)
             self._dig_dict['lpa'] = np.array([self._dig_dict['lpa']], float)
+            if self._dig_dict['hpi'] is None:
+                self._dig_dict['hpi'] = np.zeros((1, 3))
+                self._hpi_weight = 0

     def _setup_bem(self):
         # find high-res head model (if possible)

Best, Luke

bloyl avatar Oct 28 '21 23:10 bloyl

The new gui also has a small backward compatibility issue.

The old mayavi GUI supported fif files that only contained dig points.

https://github.com/mne-tools/mne-python/blob/b579d328cb67d6a0096ff5ba74e4e2e557e84065/mne/gui/_file_traits.py#L310-L318

The new gui has replaced this with a read_meas_info call

https://github.com/mne-tools/mne-python/blob/dac4f16903171c5b8a03b1a35b9c99e7bb753e25/mne/gui/_coreg.py#L335-L336

which crashes out on these files.

ValueError: Channel information not defined
Fatal Python error: Aborted

Let me know if you'd like a PR with these two bug fixes, I just don't want to complicate stuff you are already working on.

bloyl avatar Oct 29 '21 15:10 bloyl

I'd say go for it @bloyl !

larsoner avatar Oct 29 '21 15:10 larsoner

@GuillaumeFavelier given that we plan to cut 0.24 next week (preferably early), do you think the list above marked for 0.24 is realistic?

larsoner avatar Oct 29 '21 17:10 larsoner

Indeed, early next week is unrealistic for all of those.

GuillaumeFavelier avatar Oct 29 '21 17:10 GuillaumeFavelier

Okay, I'll bump those to 1.0. I think it's okay if a few of these features lag

larsoner avatar Oct 29 '21 17:10 larsoner

I am not able to get past this:

from mne.coreg import Coregistration

it throws error: cannot import name 'Coregistration' from 'mne.coreg'

The other function from mne.coreg work well like getting fiducials from fiducials = mne.coreg.get_mni_fiducials(subject, subjects_dir=subjects_dir) but this import Coregistration from mne.coreg fails for some reason. Can someone help me resolve this?

hbk008 avatar Nov 13 '21 18:11 hbk008

from mne.coreg import Coregistration

To start up the coregistration GUI, you need to call

mne.gui.coregistation()

hoechenberger avatar Nov 13 '21 18:11 hoechenberger

Yes, but I want to use automated coregistration as explained in MNE tutorial: https://mne.tools/dev/auto_tutorials/forward/25_automated_coreg.html

I have more than 1000 subjects and cannot use GUI to generate trans file every time. So, need to automate this as per above tutorial. So, how should I resolve this error?

hbk008 avatar Nov 13 '21 20:11 hbk008

@hbk008 I cannot reproduce this with MNE-Python 0.24. Please update to MNE-Python 0.24, then it should work.

Please note that this issue tracker is intended for bug reports and development issue tracking only. For usage questions like yours, please refer to our forum at https://mne.discourse.group

hoechenberger avatar Nov 13 '21 20:11 hoechenberger

Okay, will post in the forum next time. You meant update from 0.23 to 0.24, right? Anyway, I updated to 0.24 and it worked finally, thanks!

hbk008 avatar Nov 13 '21 20:11 hbk008

Great to hear it works now!! 👍

hoechenberger avatar Nov 13 '21 20:11 hoechenberger

I removed the item:

  • [ ] add support for a zoom parameter

assuming that it's among the parameters (head_inside, guess_mri_subject, ...etc) that we do not need.

GuillaumeFavelier avatar Mar 07 '22 15:03 GuillaumeFavelier