MRG: Add subject warping #3613
MRG: Add subject warping #3613
Conversation
Current coverage is 87.40% (diff: 97.92%)@@ master #3613 diff @@
==========================================
Files 343 343
Lines 60754 60890 +136
Methods 0 0
Messages 0 0
Branches 9302 9308 +6
==========================================
+ Hits 53084 53222 +138
+ Misses 4913 4912 -1
+ Partials 2757 2756 -1
|
larsoner
force-pushed
the
warp
branch
2 times, most recently
from
a2d833f to
f28f09d
Compare
larsoner
changed the title
|
Okay @agramfort I think I have this working. Do you want me to clean up the code and improve tests, or actually implement something like |
|
...if you want to review this as-is, I could at least make some hopefully helpful/explanatory image. |
|
images are always nice :)
I'll have a look over the next days
|
|
@agramfort should I add |
|
fine with me. On Wed, Oct 12, 2016 at 6:51 PM, Eric Larson [email protected]
|
|
This looks great Eric, off course I am hitting some cockameme QT import error on my OSX machine while running the example. Here is the trace:
|
|
@Eric89GXL the import error from above seems to be restricted to my OSX machine. Example works on my linux box. I just need to figure out what is going on now. |
| :toctree: generated/ | ||
| :template: class.rst | ||
|
|
||
| SphericalHarmonicTPSWarp |
There was a problem hiding this comment.
I would like to avoid acronyms in name.
How about:
SphereSplineWarp
SphericalSplineWarp
?
| @@ -17,6 +17,8 @@ Changelog | |||
|
|
|||
| - Add filter plotting functions :func:`mne.viz.plot_filter` and :func:`mne.viz.plot_ideal_filter` as well as filter creation function :func:`mne.filter.create_filter` by `Eric Larson`_ | |||
|
|
|||
| - Add TPS warping with spherical harmonic surface approximations in :class:`mne.transforms.SphericalHarmonicTPSWarp` by `Eric Larson`_ | |||
| fsaverage_trans = mne.read_trans(op.join(fsaverage_path, | ||
| 'fsaverage-trans.fif')) | ||
| for surf in fsaverage_surfs: | ||
| mne.surface.transform_surface_to(surf, 'head', fsaverage_trans) |
There was a problem hiding this comment.
it's weird that these functions work inplace no?
There was a problem hiding this comment.
yeah it is abnormal. Worth the deprecation cycle to get to copy=True default or do you want to live with copy=False default (which I will add)
| for surf in sample_surfs: | ||
| surf['rr'] = warp.transform(surf['rr']) | ||
| # recompute our normals (only used for viz here) | ||
| mne.surface.complete_surface_info(surf) |
There was a problem hiding this comment.
same remark about inplace changes. we should avoid this for public functions. wdyt?
| Kind of digitization points to use in the fitting. These can be any | ||
| combination of ('cardinal', 'hpi', 'eeg', 'extra'). Can also | ||
| be 'auto' (default), which will use only the 'extra' points if | ||
| enough are available, and if not, uses 'extra' and 'eeg' points. |
There was a problem hiding this comment.
can you explicit what enough means here?
| @@ -112,6 +112,7 @@ | |||
| op.join('data', 'FreeSurferColorLUT.txt'), | |||
| op.join('data', 'image', '*gif'), | |||
| op.join('data', 'image', '*lout'), | |||
| op.join('data', 'fsaverage', '*.fif'), | |||
There was a problem hiding this comment.
new files are pushed to pipy?
no need to update manifest.in ?
There was a problem hiding this comment.
there is a change in manifest.in, it's the first line of changes in the comparison...?
|
Thanks for the review, comments addressed @agramfort. Only remaining issue is if we should deprecate the implicit |
|
yes +1 for deprecation cycle.
regarding name after sleeping over it SphericalSplineWarp I think
it's a misnomer as it suggests it's splines on the sphere like we do
for example for EEG topographies and EEG channel interpolation.
How about Spline3dWarp ?
|
|
The problem with that is that the TPS warp itself is a spline warp (and
there is a class for that pure operation), what this does is an
approximation/smoothing followed by spline warp. So maybe SmoothSplineWarp?
Actually I think having TPS in the name like SmoothTPSWarp might be okay
because the papers talk about TPS, but it does involve another TLA.
We could also go less specific with something like SmoothedSurfaceWarp or
SphericalSurfaceWarp (because it does rely on the surfaces being low-order
sphere-like).
|
|
I would go for On Sun, Oct 16, 2016 at 3:16 PM +0200, "Eric Larson" [email protected] wrote: The problem with that is that the TPS warp itself is a spline warp (and there is a class for that pure operation), what this does is an approximation/smoothing followed by spline warp. So maybe SmoothSplineWarp? Actually I think having TPS in the name like SmoothTPSWarp might be okay because the papers talk about TPS, but it does involve another TLA. We could also go less specific with something like SmoothedSurfaceWarp or SphericalSurfaceWarp (because it does rely on the surfaces being low-order sphere-like). — |
| @@ -926,7 +928,7 @@ def _col_norm_pinv(x): | |||
|
|
|||
|
|
|||
| def _sq(x): | |||
| """Helper to square""" | |||
| """Square quickly.""" | |||
| return x * x | |||
There was a problem hiding this comment.
This is weird. Is this used somewhere?
There was a problem hiding this comment.
How about import numpy.square as _sq?
There was a problem hiding this comment.
It is actually a little bit slower:
>>> timeit.timeit('_sq(x)', setup='import numpy as np; x = np.random.RandomState(0).rand(1000); _sq = lambda x: x * x')
1.4139869213104248
>>> timeit.timeit('np.square(x)', setup='import numpy as np; x = np.random.RandomState(0).rand(1000); _sq = lambda x: x * x')
1.6284921169281006
|
Yes, in long expressions. git grep and you will see them
|
|
+1 for merge when CIs are happy. thx @Eric89GXL |
|
CIs are happy |
|
Fixed |
|
thanks @Eric89GXL ! |
|
Now onto actual use cases :)
|
#3613 breaks .elp reading by doing a transpose at the wrong time. This fixes it.
This adds support for warping between subjects using spherical harmonics. The 0th order case is fitting spheres for two sets of points and using translation plus radius-scaling to transform; order 1 uses something like an oblate spheroid, etc.
This can create stuff like this (the stuff of Matti-related nightmares), for order 4 trying to fit a true surface using just dig points (orig surface is gray, dig in dark gray, spherical transformation/deformation in purple, resulting surface in green; [code in a gist](updated 9/29)(https://gist.github.com/Eric89GXL/75f4ee8ee930f5e5bc47cff9d7f0e8e3))I still need to cross-check with other packages that the transformation is correct. But at leastorder=0should be okay, and gives us a way to tackle #2995 and eventually #3527. cc @kambysese these is some progress at least :)Closes #3611.
Closes #3527.