ENH: Add patch info to source space #1118
Conversation
| @@ -1622,6 +1640,16 @@ def add_source_space_distances(src, dist_limit=np.inf, n_jobs=1, verbose=None): | |||
| shape=(s['np'], s['np']), dtype=np.float32) | |||
| s['dist'] = d | |||
| s['dist_limit'] = np.array([dist_limit], np.float32) | |||
|
|
|||
| # Let's see if our distance was sufficient to allow for patch info | |||
| if not any(np.any(np.isinf(md)) for md in min_dists): | |||
There was a problem hiding this comment.
I remember that using generators with any / all can produce weird bugs. Better use a list comprehension here.
|
Minor comment addressed, and rebased to deal with |
|
@Eric89GXL out of curiosity, I've never used the patch feature on purpose. Do you see any differences in your results with patch information? |
|
Nope, but Matti spent some effort adding the patch information in inverse calculation (and so did we), so I figured I might as well add it here since I assume it's better. @agramfort or @mluessi do you have opinions? |
|
@Eric89GXL this was more some sort of poll because I really don't have any idea about the consequences of adding patch information. I'm absolutely +1 for adding this. Would be great to hear some thoughts from someone who has experience with it. |
| @@ -16,14 +17,15 @@ | |||
| requires_freesurfer, run_subprocess, | |||
| requires_mne, requires_scipy_version) | |||
| from mne.surface import _accumulate_normals, _triangle_neighbors | |||
|
|
|||
| from scipy.spatial.distance import cdist | |||
| from mne.externals.six.moves import zip | |||
There was a problem hiding this comment.
@Eric89GXL out of curiosity, why do you need a py2/3 safe zip here? It seems you only loop over it, which means the zip will be unpacked in python 3.
There was a problem hiding this comment.
I just always default to using the one from externals, I didn't realize there were situations where you could still use the built-in one.
There was a problem hiding this comment.
Ok, good to know I did not miss anything. The point is that zip in python 3k doesn't return a collection, but a generator. If you loop over a zip expression you're good. But things like numbers = zip(*[(1, 'a'), (2, 'b')])[0] would fail and require explicit unpacking or an additional list call.
There was a problem hiding this comment.
I have a failing test on my box:
======================================================================
FAIL: Test adding distances to source space
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/alex/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/nose/case.py",
line 197, in runTest
self.test(*self.arg)
File
"/Users/alex/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/numpy/testing/decorators.py",
line 146, in skipper_func
return f(*args, **kwargs)
File
"/Users/alex/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/numpy/testing/decorators.py",
line 146, in skipper_func
return f(*args, **kwargs)
File "/Users/alex/work/src/mne-python/mne/tests/test_source_space.py",
line 125, in test_add_source_space_distances
assert_array_equal(so['dist_limit'], np.array([-0.007], np.float32))
File
"/Users/alex/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/numpy/testing/utils.py",
line 718, in assert_array_equal
verbose=verbose, header='Arrays are not equal')
File
"/Users/alex/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/numpy/testing/utils.py",
line 644, in assert_array_compare
raise AssertionError(msg)
AssertionError:
Arrays are not equal
(mismatch 100.0%)
x: array(None, dtype=object)
y: array([-0.007], dtype=float32)
There was a problem hiding this comment.
This code doesn't touch that line... do you have this failure on master, and is your sample dataset updated?
There was a problem hiding this comment.
oups ... yes I have this error on master too due to a change in my sample
data :-/
sorry.
There was a problem hiding this comment.
It's cool, I run into this issue occasionally somehow, too.
|
LGTM shall I merge? are you done here? |
|
Yep, good to go. In the future we might want to replace that dummy source space I made with one where the C code can calculate distances (it currently segfaults!). It would allow us to increase our test coverage on Travis. It might even be possible to have a mini set of data -- using a small source space and decimated head surfaces could actually make it fairly small in my estimation. But that would be for a future PR when someone is really bored... |
ENH: Add patch info to source space
|
thanks ! |
|
BTW @agramfort do you have an opinion on how much the patch statistics help the inverse? I'm planning on adding them now because it's pretty easy to do so, but I'm just curious what you think. |
|
I never compared. So let me know :) |
Should be ready for review.