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

NF: Function to sample perpendicular directions relative to a given vector #674

Merged
merged 14 commits into from Jul 20, 2015

Conversation

Projects
None yet
3 participants
@RafaelNH
Contributor

RafaelNH commented Jul 9, 2015

Hello!

I implemented a function to sample perpendicular directions relative to a given vector. I will need this function for the future steps of the development of the diffusion kurtosis module (work in progress at #664), however since this function is quite general and might be useful for other techniques, I decided to submit it in a different pull request. I am suggesting to have this function in dipy.core.geometry. The function is being tested in the relevant testing file.

For the math details of the function, you can see the following post:

http://gsoc2015dipydki.blogspot.it/2015/07/rnh-post-8-computing-perpendicular.html

@@ -239,7 +254,42 @@ def test_compose_decompose_matrix():
assert_array_almost_equal(scale, sc)
def test_perpendicular_directions():
num = 35
vector_v = sphere2cart(1, np.pi/4, np.pi/4)

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

Maybe a stronger test-case would be to generate a few vectors randomly?

# check case of vector_v is aligned to x_axis
vector_v = [1., 0., 0.]
pd = perpendicular_directions(vector_v, num=num, half=False)
for d in pd:

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

To reduce repetition in this test, you could write it as a loop:

for vv in [sphere2cart(1, np.pi/4, np.pi/4), [1., 0., 0.]]:
    ...
@@ -906,3 +910,57 @@ def compose_transformations(*mats):
return prev
def perpendicular_directions(v, num=30, half=False):
r""" Computes n equaly spaced perpendicular directions relative to a given

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

"equaly" => "equally"

num : int, optional
Number of perpendicular directions to generate
half : bool, optional
If haph is True, perpendicular directions are sampled on halp of the

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

"haph" => "half"

Number of perpendicular directions to generate
half : bool, optional
If haph is True, perpendicular directions are sampled on halp of the
unit circunference perpendicular to v, otherwive perpendicular

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

"circunference" => "circumference"

half : bool, optional
If haph is True, perpendicular directions are sampled on halp of the
unit circunference perpendicular to v, otherwive perpendicular
directions are sampled on the full circunference. Defaut of half is

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

same here

This comment has been minimized.

@arokem

arokem Jul 11, 2015

Member

"Defaut" => "Default"

@arokem

This comment has been minimized.

Member

arokem commented Jul 11, 2015

Some small comments, but other than that, it looks good to me.

I don't know how I feel about having the documentation of the function in a
separate website (your blog). Maybe worth adding a more complete
explanation of the function to a Notes section of the docstring.

On Thu, Jul 9, 2015 at 12:06 PM, Rafael Neto Henriques <
notifications@github.com> wrote:

Hello!

I implemented a function to sample perpendicular directions relative to a
given vector. I will need this function for the future steps of the
development of the diffusion kurtosis module (work in progress at #664
#664), however since this function is
quite general and might be useful for other techniques, I decided to submit
it in a different pull request. I am suggesting to have this function in
dipy.core.geometry. The function is being tested in the relevant testing
file.

For the math details of the function, you can see the following post:

http://gsoc2015dipydki.blogspot.it/2015/07/rnh-post-8-computing-perpendicular.html

You can view, comment on, or merge this pull request online at:

#674
Commit Summary

  • NF: Implemented a function that computes n equaly spaced
    perpendicular directions given a reference vector v

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#674.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Jul 13, 2015

Hi @arokem! I think I answered all your suggestions in my last commit. Let me know if there is anything else that needs improvement! Anyone else have comments to this pull request?

@RafaelNH RafaelNH force-pushed the RafaelNH:perpendicular_directions branch from 7fd38dc to db629aa Jul 14, 2015

RafaelNH added some commits Jul 14, 2015

BF: Bug was caused by code in test. Basically when checking if direct…
…ions are sampled by multiples of 2*pi / num, I was including the comparison between the first perpendicular vector with itself. In a world with perfect percision, this would not be a problem since np.dot(d, d)=1 and np.arccos(1)=0. However, the percision of np.dot can result in values slightly larger than 1, like 1.000000000000002. In this cases, np.arccos will give NaN. To solve thisissue I remove the comparison between the selected ref perpendicular direction with itself.
@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Jul 14, 2015

Bug fixed! Anyone else have comments to this pull request? Can we merge it?

For more details see:
http://gsoc2015dipydki.blogspot.it/2015/07/rnh-post-8-computing-perpendicul
ar.html

This comment has been minimized.

This comment has been minimized.

@RafaelNH

RafaelNH Jul 15, 2015

Contributor

Done =)!

@arokem

This comment has been minimized.

Member

arokem commented Jul 14, 2015

LGTM. I'll let others pitch in, or merge sometime early next week.

@arokem

This comment has been minimized.

arokem commented on dipy/core/geometry.py in 6f5e2ed Jul 15, 2015

Small comment. I would replace this with:

For details on this calculation, see `here <...>`_. 

Also: is there a line-break in the middle of that link? I don't think it would work, so please don't wrap this line to 80 chars.

@arokem

This comment has been minimized.

Member

arokem commented Jul 15, 2015

Yup - ready from my point of view. I'd be really happy if someone else took a look. There are possibly failure modes that I am not thinking about.

@arokem

This comment has been minimized.

Member

arokem commented Jul 20, 2015

Since no one is complaining, here goes.

arokem added a commit that referenced this pull request Jul 20, 2015

Merge pull request #674 from RafaelNH/perpendicular_directions
NF: Function to sample perpendicular directions relative to a given vector

@arokem arokem merged commit 07b81b3 into nipy:master Jul 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@coveralls

This comment has been minimized.

coveralls commented Apr 3, 2017

Coverage Status

Changes Unknown when pulling e52b2db on RafaelNH:perpendicular_directions into ** on nipy:master**.

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