mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.66k stars 1.31k forks source link

ENH: New interface for Coregistration #8833

Open GuillaumeFavelier opened 3 years ago

GuillaumeFavelier commented 3 years ago

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*)
larsoner commented 2 years ago

@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.

GuillaumeFavelier commented 2 years ago

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

bloyl commented 2 years ago

@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 commented 2 years ago

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.

larsoner commented 2 years ago

I'd say go for it @bloyl !

larsoner commented 2 years ago

@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?

GuillaumeFavelier commented 2 years ago

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

larsoner commented 2 years ago

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

hbk008 commented 2 years ago

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?

hoechenberger commented 2 years ago

from mne.coreg import Coregistration

To start up the coregistration GUI, you need to call

mne.gui.coregistation()
hbk008 commented 2 years ago

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?

hoechenberger commented 2 years ago

@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

hbk008 commented 2 years ago

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!

hoechenberger commented 2 years ago

Great to hear it works now!! 👍

GuillaumeFavelier commented 2 years ago

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.