[MRG] Coregistration #3466

christianbrodbeck · 2016-07-27T23:34:00Z

@jona-sassenhagen the function is mne.coreg.coregister_fiducials. It currently has a scale parameter to use it with fsaverage, but this does not lead to satisfactory results and should be removed before merge.

The PR also contains some other small modifications.

Allow viz.plot_trans to plot fiducials
Allow scaling MRI without label files

codecov-io · 2016-07-28T01:06:02Z

Codecov Report

Merging #3466 into master will decrease coverage by -0.1%.
The diff coverage is 76.92%.

@@            Coverage Diff            @@
##           master    #3466     +/-   ##
=========================================
- Coverage   86.07%   85.97%   -0.1%     
=========================================
  Files         350      350             
  Lines       63028    63209    +181     
  Branches     9639     9659     +20     
=========================================
+ Hits        54251    54344     +93     
- Misses       6097     6181     +84     
- Partials     2680     2684      +4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a02e8...3851698. Read the comment docs.

larsoner · 2017-02-14T14:41:00Z

@christianbrodbeck is this worth resurrecting? It seems like at the very least the MRI fiducials would be cool.

christianbrodbeck · 2017-02-14T15:12:38Z

Sure! I can rebase or split it up if you think some features require more work than others.

larsoner

Otherwise LGTM

larsoner · 2017-02-14T15:24:24Z

mne/transforms.py

-        raise TypeError('cf must be str or int, not %s' % type(cf))
+    else:
+        try:
+            cf = int(cf)


you can instead do from numbers import Integral; if not isinstance(cf, Integral):

I wish I'd known this! Simply returning int(cf) (which is in master now) is sufficient though right?

Actually I think we already have a function like this somewhere that converts coordinate frame str or int to int. Can you check around, probably in mne/transforms.py?

... oh wait, this is in transforms. Never mind! Yeah int(cf) is fine

larsoner · 2017-02-14T15:25:27Z

mne/viz/_3d.py

    ref_meg : bool
        If True (default False), include reference MEG sensors.
+    fiducials : bool | str


this should probably be mri_fiducials. The digitized fiducials are already displayed as part of dig

I'll change. i assume also add the .. versionadded:: 0.14 tag?

larsoner · 2017-02-14T15:26:06Z

mne/viz/_3d.py

+            head_dev_t = invert_transform(info['dev_head_t'])
+            mri_dev_t = combine_transforms(mri_head_t, head_dev_t, 'mri',
+                                           'meg')
+            fid_locs = apply_trans(mri_dev_t, fid_locs)


This can now be simplified because there is IIRC a mri_transform that is used to take anything originally in MRI space to the destination space

8abb42b of #3999

larsoner · 2017-02-14T15:27:13Z

mne/coreg.py

+                         "frames")
+    coord_frame_from = frames_from[0]
+    coords_to = _fiducial_coords(fiducials, coord_frame_to)
+    trans = fit_matched_points(coords_from, coords_to, tol=tol)


I think this probably isn't the correct function. From fiducials you get a center point and axis directions. Using that geometry analytically is better than using a generic fitting function

larsoner · 2017-02-15T19:15:27Z

mne/gui/_fiducials_gui.py

+                'r': np.array(self.nasion[0])},
+               {'kind': FIFF.FIFFV_POINT_CARDINAL,
+                'ident': FIFF.FIFFV_POINT_RPA,
+                'r': np.array(self.rpa[0])}]
        write_fiducials(fname, dig, FIFF.FIFFV_COORD_MRI)


It would be good at some point to use mne.io.write_dig instead. This is just a specific use case of it.

Hm write_fiducials() does nothing but call write_dig()... should it be deprecated?

Yes probably. I think back when I fixed up write_dig I just wasn't sure how to fix the coreg code not to use it anymore.

larsoner · 2017-02-22T19:55:31Z

This could use a rebase (might be easier to start from scratch?) then should be good hopefully

christianbrodbeck · 2017-02-22T21:27:38Z

Well the only thing that's left is the actual coregistration which as you indicated should be rewritten with the analytic approach...

larsoner · 2017-02-22T21:28:34Z

For now I think we can keep that. I think that we'll want to write a separate "mode" entirely, e.g. coordinate-frame-center-based instead of nasion-based. It's what I would have expected going in.

christianbrodbeck · 2017-02-22T22:30:24Z

Rebased and added a test; This is not nasion based, it just finds a transform to minimize points distances.

christianbrodbeck · 2017-02-23T00:32:20Z

mne/transforms.py

@@ -63,8 +63,6 @@ def _to_const(cf):
        if cf not in _str_to_frame:
            raise ValueError('Unknown cf %s' % cf)
        cf = _str_to_frame[cf]
-    elif not isinstance(cf, (int, np.integer)):
-        raise TypeError('cf must be str or int, not %s' % type(cf))
    return int(cf)


I think this check is superfluous, or am I missing some possibility?

I think it prevents people from accidentally passing float. The correct thing to do, though, is really to check against from numbers import Integral; if isinstance(x, Integral)

christianbrodbeck · 2017-02-24T15:53:01Z

Looks like numpy.int32 does not qualify as Integral in one configuration? https://travis-ci.org/mne-tools/mne-python/jobs/204443245#L2868

larsoner · 2017-02-24T15:57:31Z

Argh, probably an old numpy issue :( Not sure the best way to handle it. Maybe isinstance(..., (Integral, np.int32)) for backward compatibility. We really shouldn't get anything other than int, np.int32, or np.int64 (maybe) anyway.

christianbrodbeck · 2017-02-24T16:17:17Z

Actually I get an error in this test (https://github.com/mne-tools/mne-python/blob/master/mne/tests/test_coreg.py#L67) and I am not sure how it could run without error?

    src = mne.read_source_spaces(path)
    mne.add_source_space_distances(src)
    src.save(path)

larsoner · 2017-02-24T16:25:32Z

Looks like you fixed it already, yes?

christianbrodbeck · 2017-02-24T17:39:09Z

yes :)

larsoner · 2017-02-24T17:44:19Z

Okay great. This looks pretty simple so I'll merge once the CIs come back happy

larsoner · 2017-02-24T19:57:50Z

Travis is unhappy with style

https://travis-ci.org/mne-tools/mne-python/jobs/205022035#L3106

larsoner · 2017-02-24T20:39:13Z

Thanks @christianbrodbeck

christianbrodbeck mentioned this pull request Jul 27, 2016

Transform docs and API #3465

Closed

christianbrodbeck force-pushed the coreg branch from 586f797 to 10a90cc Compare August 23, 2016 20:10

larsoner reviewed Feb 14, 2017

View reviewed changes

larsoner reviewed Feb 15, 2017

View reviewed changes

christianbrodbeck mentioned this pull request Feb 16, 2017

MRG+1: plot_trans: add MRI fiducials #3999

Merged

christianbrodbeck force-pushed the coreg branch 2 times, most recently from 42e5475 to dde8ed3 Compare February 22, 2017 22:02

christianbrodbeck changed the title ~~[WIP] Coregistration~~ [MRG] Coregistration Feb 23, 2017

christianbrodbeck commented Feb 23, 2017

View reviewed changes

christianbrodbeck added 3 commits February 22, 2017 21:00

ENH: coregister_fiducials()

b227388

TEST with known trans

a338016

Transform: type-check with numbers.Integral

35e2202

christianbrodbeck force-pushed the coreg branch from c591be3 to 35e2202 Compare February 23, 2017 02:03

STY

6c392d1

christianbrodbeck added 2 commits February 24, 2017 10:59

add numpy.int32 to int check

1e85fbc

TEST: enable test that does not require mne anymore

ea5dc90

fix test

3851698

STY

5192d53

larsoner merged commit 31c2c28 into mne-tools:master Feb 24, 2017

christianbrodbeck deleted the coreg branch February 27, 2017 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Coregistration #3466

[MRG] Coregistration #3466

christianbrodbeck commented Jul 27, 2016

codecov-io commented Jul 28, 2016 •

edited

Loading

larsoner commented Feb 14, 2017

christianbrodbeck commented Feb 14, 2017

larsoner left a comment

larsoner Feb 14, 2017

christianbrodbeck Feb 15, 2017

larsoner Feb 15, 2017

larsoner Feb 15, 2017

larsoner Feb 14, 2017

christianbrodbeck Feb 16, 2017

larsoner Feb 16, 2017

larsoner Feb 14, 2017

christianbrodbeck Feb 16, 2017

larsoner Feb 14, 2017

larsoner Feb 15, 2017

christianbrodbeck Feb 16, 2017

larsoner Feb 16, 2017

larsoner commented Feb 22, 2017

christianbrodbeck commented Feb 22, 2017

larsoner commented Feb 22, 2017

christianbrodbeck commented Feb 22, 2017

christianbrodbeck Feb 23, 2017

larsoner Feb 23, 2017

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

larsoner commented Feb 24, 2017

larsoner commented Feb 24, 2017

[MRG] Coregistration #3466

[MRG] Coregistration #3466

Conversation

christianbrodbeck commented Jul 27, 2016

codecov-io commented Jul 28, 2016 • edited Loading

Codecov Report

larsoner commented Feb 14, 2017

christianbrodbeck commented Feb 14, 2017

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner commented Feb 22, 2017

christianbrodbeck commented Feb 22, 2017

larsoner commented Feb 22, 2017

christianbrodbeck commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

christianbrodbeck commented Feb 24, 2017

larsoner commented Feb 24, 2017

larsoner commented Feb 24, 2017

larsoner commented Feb 24, 2017

codecov-io commented Jul 28, 2016 •

edited

Loading