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

flexible grid to streamline affine generation and pathlength function #1114

Merged
merged 26 commits into from Oct 28, 2016

Conversation

Projects
None yet
6 participants
@kesshijordan
Contributor

kesshijordan commented Sep 2, 2016

Included are two related additions :

NF: The path_length function computes the shortest path, along any streamline, between the roi and each voxel. @MrBago and I are using this for a particular project we're working on, but it could be useful in other applications.

NF: The other supporting functions (flexi tvis affine, etc.) are used in path_length, but also address a problem that I've found particularly difficult as a going-up-the-learning-curve user: mapping between grid and streamline spaces with different voxel orders. After the change to using affines instead of providing voxel sizes to functions like target, one has to sort through a lot of information in the headers to get everything in the same space. These flexi tvis affine functions are extremely helpful for painless targeting and fvtk renderings.

def flexi_tvis_affine(sl_vox_order, grid_affine, dim, voxel_size):
""" Computes the mapping from voxel indices to streamline points,
reconciling streamlines and grids with different voxel orders

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

indent to the left

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

What do you mean? I think the def is against the left edge, and the docstring is indented like the rest of the functions in this file?

Parameters
----------
sl_vox_order: string of length 3

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

add space after parameter and before :
please change everywhere
it should be sl_vox_order : string otherwise the docstring doesn't render well.

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

fixed

----------
sl_vox_order: string of length 3
a string that describes the voxel order of the streamlines (ex: LPS)
grid_affine: nii_aff: array (4, 4),

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

two parameters?

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

I'm interpreting this as "why do you include both the sl_vox_order and the grid_affine as parameters?" The streamline voxel order doesn't necessarily match that of the affine of the underlying grid one is trying to target/display in fvtk... the function called here (get_flexi_tvis_affine) converts both to orientations, then reconciles them with an affine. The orientations can be different, so must be pulled from both the nifti and the trk headers.

a string that describes the voxel order of the streamlines (ex: LPS)
grid_affine: nii_aff: array (4, 4),
An affine matrix describing the current space of the grid in relation to RAS+ scanner space
dim: tuple of length 3

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

add space (as previously suggested)

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

fixed

An affine matrix describing the current space of the grid in relation to RAS+ scanner space
dim: tuple of length 3
dimension of the grid
voxel_size: array (3,0)

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

fixed

Returns
-------
flexi_tvis_aff: this affine maps between a grid and a trackvis space

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

same same

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

fixed fixed

Parameters
----------
tvis_hdr: header from a trackvis file

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 11, 2016

Member

same... (change everywhere in your files)

This comment has been minimized.

@kesshijordan

kesshijordan Sep 13, 2016

Contributor

fixed

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 11, 2016

Also you may want to look at the new streamline API available in DIPY. This should solve many different problems with dealing with streamline datasets.

@arokem

This comment has been minimized.

Member

arokem commented Sep 11, 2016

On Sat, Sep 10, 2016 at 9:43 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Also you may want to look at the new streamline API available in DIPY.
This should solve many different problems with dealing with streamline
datasets.

Do you mean the new streamline API in nibabel? Worth taking a look at
that, for sure:
http://nipy.org/nibabel/reference/nibabel.streamlines.html#module-nibabel.streamlines


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1114 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNlX4a1k_dZpquyOK1GRhzxf34EiPks5qo4bogaJpZM4JzTI7
.

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Sep 13, 2016

Thank you for the comments. I have fixed most of the formatting issues (not sure I know what you mean regarding indentation), and commented on the number of parameters question. I also noticed that there are supposed to be two lines between each function, so I corrected that, as well. I will check out the nibabel API to see if that fixes some of these issues.

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2016

Hey @kesshijordan : do you understand why these test failures are happening?

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Sep 29, 2016

Hi, @arokem. Thanks for checking in. I think I figured it out: I found errors I recognized when I scrolled to the bottom of the Travis build outputs. I addressed them, resubmitted to Travis, and only one build failed (#1.5; I think because of the numpy version) "AttributeError: 'numpy.ufunc' object has no attribute 'at'", referring to a use of np.minimum.at(a,(i,j,k),b)... so I replaced it with a[i,j,k]=np.minimum(a[i,j,k],b) so that all of the builds work. Is there anything else I should be looking for besides the big ERRORs?

@codecov-io

This comment has been minimized.

codecov-io commented Sep 29, 2016

Current coverage is 80.96% (diff: 99.24%)

Merging #1114 into master will increase coverage by 0.09%

@@             master      #1114   diff @@
==========================================
  Files           217        217          
  Lines         24593      24717   +124   
  Methods           0          0          
  Messages          0          0          
  Branches       2491       2499     +8   
==========================================
+ Hits          19888      20011   +123   
  Misses         4194       4194          
- Partials        511        512     +1   

Powered by Codecov. Last update da31980...a59c50f

@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2016

Coverage Status

Coverage increased (+0.07%) to 83.303% when pulling a78e5e0 on kesshijordan:master into 994d43f on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 29, 2016

Coverage Status

Coverage increased (+0.07%) to 83.303% when pulling a78e5e0 on kesshijordan:master into 994d43f on nipy:master.

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Sep 30, 2016

@MrBago just pointed out that substituting a[i,j,k]=np.minimum(a[i,j,k],b) for np.minimum.at(a,(i,j,k),b) introduces errors and recommended fixing it with a little function that replaces np.minimum.at if the numpy version is less than 8.

@coveralls

This comment has been minimized.

coveralls commented Sep 30, 2016

Coverage Status

Coverage decreased (-0.4%) to 82.814% when pulling 6abccfc on kesshijordan:master into 994d43f on nipy:master.

MrBago and others added some commits Oct 1, 2016

Merge pull request #1 from MrBago/bf_minat
BF - fix _min_at function
@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.08%) to 83.318% when pulling 2dfaa4c on kesshijordan:master into 994d43f on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2016

Coverage Status

Coverage increased (+0.08%) to 83.318% when pulling 2dfaa4c on kesshijordan:master into 994d43f on nipy:master.

@arokem

Sorry that this has languished. The code looks good to me now, and is almost ready to merge. I had just a few small formatting suggestions.

sl_vox_order : string of length 3
a string that describes the voxel order of the streamlines (ex: LPS)
grid_affine : array (4, 4),
An affine matrix describing the current space of the grid in relation to RAS+ scanner space

This comment has been minimized.

@arokem

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

Line's too long, I believe.

sl_ornt = orientation_from_string(str(sl_vox_order))
grid_ornt = nib.io_orientation(grid_affine)
reorder_grid = reorder_voxels_affine(grid_ornt, sl_ornt, np.array(dim)-1, np.array([1,1,1]))

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

Here as well: line is longer than 80 characters (PEP8)

----------
tvis_hdr : header from a trackvis file
nii_aff : array (4, 4),
An affine matrix describing the current space of the grid in relation to RAS+ scanner space

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

PEP8 (line length)

nii_aff : array (4, 4),
An affine matrix describing the current space of the grid in relation to RAS+ scanner space
nii_data : nd array
3D array, each with shape (x, y, z) corresponding to the shape of the brain volume,

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

Period instead of comma at the end of the sentence?

origin = np.dot(affine, [0, 0, 0, 1])
assert_array_almost_equal(origin[:3], np.multiply(dim,voxel_size)-voxel_size/2)

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

One more linebreak here (PEP8)

trk_point = np.dot(affine, np.append(vox_point, 1))
assert_array_almost_equal(trk_point[:3], (vox_point[[1, 2, 0]] + 0.5) * vsz)

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

One more linebreak here (PEP8)

kesshijordan added some commits Oct 27, 2016

Merge branch 'master' of git://github.com/nipy/dipy
Conflicts:
	dipy/tracking/tests/test_utils.py
Merge branch 'master' of github.com:kesshijordan/dipy
Conflicts:
	dipy/tracking/tests/test_utils.py
Merge branch 'master' of https://github.com/nipy/dipy
Conflicts:
	dipy/tracking/tests/test_utils.py
Merge branch 'master' of https://github.com/kesshijordan/dipy
Conflicts:
	dipy/tracking/tests/test_utils.py
@coveralls

This comment has been minimized.

coveralls commented Oct 28, 2016

Coverage Status

Coverage increased (+0.09%) to 83.024% when pulling a59c50f on kesshijordan:master into da31980 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Oct 28, 2016

Coverage Status

Coverage increased (+0.09%) to 83.024% when pulling a59c50f on kesshijordan:master into da31980 on nipy:master.

@arokem arokem merged commit 9bbb5a4 into nipy:master Oct 28, 2016

4 checks passed

codecov/patch 99.24% of diff hit (target 80.86%)
Details
codecov/project 80.96% (+0.09%) compared to da31980
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 83.024%
Details
@arokem

This comment has been minimized.

Member

arokem commented Oct 28, 2016

Thanks! Please do consider adding documentation examples using these functions, so that we can see how exactly this is useful to you. If you need a hand with that, just let me know.

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Sep 4, 2018

Sorry this took so long, @arokem. I just made a PR for the documentation that goes with this #1631.

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