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

Deform streamlines #1398

Merged
merged 10 commits into from Apr 18, 2018

Conversation

Projects
6 participants
@conorkcorbin
Contributor

conorkcorbin commented Jan 9, 2018

Add function to dipy.tracking.streamline.py that applies deformation field to a list of streamlines. To do this we:

  1. Transform streamlines from voxmm space to grid space using transform_streamlines()
  2. Interpolate displacements onto streamlines in grid space using values_from_volume()
  3. Transform streamlines in grid space to world space using transform_streamlines()
  4. Apply displacements to streamlines in world space
  5. Transform streamlines in world space back to their original voxmm space using transform_streamlines()

streams2grid affine can be generated by taking the inverse of dipy.utils.get_flexi_tvis_affine() output.
grid2world is the affine of the warpfile.

The unit test applies a random deformation to a list of streamlines. It interpolates the displacements used and then subtracts them from the deformed streamlines to create a set of streamlines that should be the same as the original set.

@arokem

This comment has been minimized.

Member

arokem commented Jan 9, 2018

Overall, this looks great! The error currently on Travis is unrelated, and is affecting all our current PRs. Seems like a change in behavior in numpy (I think @skoudoro is looking into it).

For now, two comments:

  1. The first is about conforming to the coding style we use. I recommend running a pep8 checker on the code (like this).

  2. The second is to wonder whether the loop could be speeded up substantially by implementing it in Cython.

@codecov-io

This comment has been minimized.

codecov-io commented Jan 9, 2018

Codecov Report

Merging #1398 into master will increase coverage by <.01%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
+ Coverage    87.5%   87.51%   +<.01%     
==========================================
  Files         241      241              
  Lines       30701    30724      +23     
  Branches     3323     3326       +3     
==========================================
+ Hits        26865    26887      +22     
- Misses       3059     3060       +1     
  Partials      777      777
Impacted Files Coverage Δ
dipy/tracking/tests/test_streamline.py 98.24% <100%> (+0.05%) ⬆️
dipy/tracking/streamline.py 95.11% <80%> (-0.71%) ⬇️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 167b1a5...ab699b8. Read the comment docs.

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Jan 9, 2018

Great! Everything should now be pep8 compliant. I also implemented the loop in cython, which seems to half the runtime.

@arokem

Awesome work! I had a few small comments and questions.

@@ -89,6 +90,40 @@ def center_streamlines(streamlines):
return [s - center for s in streamlines], center
def deform_streamlines(streamlines, deformField, streams2grid, grid2world):

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

PEP8: "deformField" => "deform_field"

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

changed

List of 2D ndarrays of shape[-1]==3
deformField : 4D numpy array
x,y,z displacements stored in volume, shape[-1]==3
streams2grid : array, (4,4)

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Is this the affine from the streamlines to the acquisition grid?

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

this takes streamlines from voxmm space to image grid space. So for instance if both streamlines and the image were LAS and vox dimensions were 2,2,2 it would effectively be the transformation that divides all streamline points by 2.

cdef np.npy_intp i
for i in range(len(streamlines)):
dtype = streamlines[i].dtype
# HACK: To avoid memleaks we have to recast with astype(dtype).

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Wow. Care to elaborate?

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Oh, I see - this is copied from other parts of streamlinespeed. Maybe someone else knows what this is about?

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

yeah this was just copied from compress_streamlines.

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

I don't know exactly why it produces memleaks without the .astype :-/ but it was causing issues with large tractogram (represented as a big list of ndarrays). I did make a series of tests check for that (e.g. https://github.com/nipy/dipy/blob/master/dipy/tracking/tests/test_streamline.py#L499). My guess is Cython is not releasing the refcount when it is done with streamlines[i].

@@ -516,6 +517,46 @@ def test_unlist_relist_streamlines():
assert_array_equal(streamlines[i], streamlines2[i])
def test_deform_streamlines():
# streamlines needs to be a list of streamlines
A = streamlines = np.array([[1, 2, 3], [1, 2, 3.]])

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Why are you allocating into streamlines if you overwrite it in the following line?

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

this is a mistake, changed.

A = streamlines = np.array([[1, 2, 3], [1, 2, 3.]])
streamlines = [A for i in range(10)]
# Create Random deformation field
deformationField = np.empty((4, 4, 4, 3))

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

PEP8: deformationField => deformation_field

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

done

for i in range(shape[0]):
for j in range(shape[1]):
for k in range(shape[2]):
deformationField[i][j][k] = np.random.rand(3)

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Would you mind using randn instead? That way we get deformations in both negative in positive directions.

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

done

deformationField[i][j][k] = np.random.rand(3)
# Specify stream2grid and grid2world
stream2grid = np.eye(4)

This comment has been minimized.

@arokem

arokem Jan 10, 2018

Member

Occurs to me that you could set a default value of np.eye(4) for these two in the function itself.

This comment has been minimized.

@conorkcorbin

conorkcorbin Jan 10, 2018

Contributor

I used np.eye to avoid using transform_streamlines() in the unit test b/c it has already been tested, but you are right it probably should be tested in relation to this new function. Changed it to add a random scaling.

@arokem

arokem approved these changes Jan 10, 2018

@arokem

This comment has been minimized.

Member

arokem commented Jan 10, 2018

Looks great! Could you please rebase on top of master to integrate changes in #1400 into your branch? Then, we can make sure that this passes on Travis.

Anyone else want to take a look here?

@conorkcorbin conorkcorbin force-pushed the conorkcorbin:deform_streamlines branch from c34b4d2 to 60db661 Jan 11, 2018

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Jan 11, 2018

Great it's been rebased onto master.

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Jan 12, 2018

Travis doesn't seem to like it too much. Test failing in test_utils. Any ideas?

@arokem

This comment has been minimized.

Member

arokem commented Jan 12, 2018

Yeah - it's mysterious. I can't reproduce locally on my laptop when working off of your branch. Let me give Travis a restart. Maybe this is just some weird glitch.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 13, 2018

Yes, give me a bit of time to look at this.

@skoudoro skoudoro added this to Needs a review in Dipy 0.14.0 Jan 17, 2018

streamlines = [A for i in range(10)]
# Create Random deformation field
deformation_field = np.empty((8, 8, 8, 3))

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

You can generate a random 4D numpy arrays like this deformation_field = np.random.randn(8, 8, 8, 3).

This comment has been minimized.

@conorkcorbin

conorkcorbin Mar 22, 2018

Contributor

sounds good, changed.

def test_deform_streamlines():
# streamlines needs to be a list of streamlines
A = np.array([[1, 2, 3], [1, 2, 3.]])
streamlines = [A for i in range(10)]

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

What happens if your streamlines don't have all the same number of points? You could use one of the global variables defined at the top of this test file (e.g. streamlines define at line 152).

This comment has been minimized.

@conorkcorbin

conorkcorbin Mar 22, 2018

Contributor

am now testing with global streamlines

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 20, 2018

@conorkcorbin can you rebase this PR please? Tests are failing here! Also you haven't responded to @MarcCote questions. We need to move one with this PR! :)

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Mar 20, 2018

@Garyfallidis hi yes I will rebase ASAP. I also need to update the function because it currently doesn't support the case where the original streamline space image grid is not equal to the image grid we are warping streamlines to. Streamlines should be returned in voxmm space of the new grid. This is done just need to commit.

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Mar 22, 2018

@Garyfallidis now updated.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 23, 2018

Travis is not happy : ImportError: cannot import name 'add_displacements' @conorkcorbin

I suppose you forgot to commit streamlinespeed.pyx ;-)

@conorkcorbin

This comment has been minimized.

Contributor

conorkcorbin commented Mar 23, 2018

@skoudoro ahh yeah I got rid of the cython... need to get rid of the import too. My bad. 

@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 4, 2018

Any other comment @MarcCote @arokem @Garyfallidis? it looks ready to go.

@arokem

This comment has been minimized.

Member

arokem commented Apr 4, 2018

👍

@MarcCote

Minor comments, otherwise LGTM.

displacements = values_from_volume(deform_field, stream_in_curr_grid)
stream_in_world = transform_streamlines(stream_in_curr_grid,
current_grid_to_world)
new_streams_in_world = list(np.add(displacements, stream_in_world))

This comment has been minimized.

@MarcCote

MarcCote Apr 5, 2018

Contributor

It's up to you but I find displacements + stream_in_world easier to read than np.add(...).

This comment has been minimized.

@conorkcorbin

conorkcorbin Apr 18, 2018

Contributor

replaced with list comprehension - adding element wise

deformation_field = np.random.randn(200, 200, 200, 3)
# shape = deformation_field.shape
# for i in range(shape[0]):
# for j in range(shape[1]):

This comment has been minimized.

@MarcCote

MarcCote Apr 5, 2018

Contributor

You can remove these commented lines.

This comment has been minimized.

@conorkcorbin

conorkcorbin Apr 18, 2018

Contributor

done

@@ -516,6 +517,56 @@ def test_unlist_relist_streamlines():
assert_array_equal(streamlines[i], streamlines2[i])
def test_deform_streamlines():
# streamlines needs to be a list of streamlines
A = np.array([[1, 2, 3], [1, 2, 3.]])

This comment has been minimized.

@MarcCote

MarcCote Apr 5, 2018

Contributor

Is this still used?

This comment has been minimized.

@conorkcorbin

conorkcorbin Apr 18, 2018

Contributor

deleted

@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 14, 2018

Hi @conorkcorbin, Can you do the small cosmetic changes address by @MarcCote.
This PR is really close to being merge and just missing this point.
Thank you

conorkcorbin added some commits Apr 18, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Apr 18, 2018

Thank you @conorkcorbin for this nice feature. merging!

@skoudoro skoudoro merged commit b1c8079 into nipy:master Apr 18, 2018

3 checks passed

codecov/patch 91.3% of diff hit (target 87.5%)
Details
codecov/project 87.51% (+<.01%) compared to 167b1a5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Dipy 0.14.0 automation moved this from Needs a review to Done Apr 18, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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