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

ENH: New interface for Coregistration #8833

Open
30 of 38 tasks
GuillaumeFavelier opened this issue Feb 3, 2021 · 15 comments
Open
30 of 38 tasks

ENH: New interface for Coregistration #8833

GuillaumeFavelier opened this issue Feb 3, 2021 · 15 comments
Assignees

Comments

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Feb 3, 2021

This PR summarizes the ideas to implement for coreg:

Milestone 1.0
  • increase fiducial picking performance (ENH: Coreg GUI #9689 (comment))
  • add support for the project_eeg parameter
  • add support for the scale_by_distance parameter
  • add support for the mark_inside parameter
  • console/status_bar idea to add to the UI whatever is printed in the terminal
  • display dist estimation in mm.
  • add units to UI (Omit more explicit message)
  • display axes
  • add tooltips as much as possible on widgets
  • add support for the scaling parameters (WIP,ENH: new coreg gui features (part 3) #10117)

Bonus territory:

Milestone 1.1
Done
Bugs
@larsoner
Copy link
Member

@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
Copy link
Contributor Author

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

@bloyl
Copy link
Contributor

bloyl commented Oct 28, 2021

@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
Copy link
Contributor

bloyl commented Oct 29, 2021

The new gui also has a small backward compatibility issue.

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

elif self.file.endswith(('.fif', '.fif.gz')):
info = None
fid, tree, _ = fiff_open(self.file)
fid.close()
if len(dir_tree_find(tree, FIFF.FIFFB_MEAS_INFO)) > 0:
info = read_info(self.file, verbose=False)
elif len(dir_tree_find(tree, FIFF.FIFFB_ISOTRAK)) > 0:
info = _empty_info(1)
info['dig'] = read_dig_fif(fname=self.file).dig

The new gui has replaced this with a read_meas_info call

def _info_file_changed(self, change=None):
self._info = read_info(self._info_file)

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
Copy link
Member

I'd say go for it @bloyl !

@larsoner
Copy link
Member

@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
Copy link
Contributor Author

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

@larsoner
Copy link
Member

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

@larsoner larsoner modified the milestones: 0.24, 1.0 Oct 29, 2021
@bloyl bloyl mentioned this issue Oct 30, 2021
@hbk008

This comment has been minimized.

@hoechenberger

This comment has been minimized.

@hbk008

This comment has been minimized.

@hoechenberger

This comment has been minimized.

@hbk008

This comment has been minimized.

@hoechenberger

This comment has been minimized.

@GuillaumeFavelier
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants