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

Generate b vectors using disperse_charges #1067

Merged
merged 3 commits into from Jun 7, 2016

Conversation

Projects
None yet
5 participants
@sahmed95
Contributor

sahmed95 commented Jun 2, 2016

A function to generate arbitary b vectors using the disperse function

@arokem

This comment has been minimized.

Member

arokem commented Jun 2, 2016

Thanks! Would be good to have a test for this.

"""Function to generate arbitary b-vectors of length N
This may be used for testing or various simulations.
It uses the disperse_charges function from dipy.core.sphere
Refer to the example gradients_spheres for details"""

This comment has been minimized.

@arokem

arokem Jun 2, 2016

Member

Docstring needs to be reformatted to the standard format

theta = np.pi * np.random.rand(N)
phi = 2 * np.pi * np.random.rand(N)
hsph_initial = HemiSphere(theta=theta, phi=phi)
hsph_updated, potential = disperse_charges(hsph_initial, 5000)

This comment has been minimized.

@arokem

arokem Jun 2, 2016

Member

This number (5000) should be a key-word argument to the function.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 2, 2016

I am wondering how can we test this ? Won't the b vectors be in arbitrary directions ?

@arokem

This comment has been minimized.

Member

arokem commented Jun 2, 2016

They will be, but there are still properties of the results that we can check. For example, for N=2, the vectors should be just about 90 degrees from each other, so you can check the angle between the resulting vectors.

@grlee77

This comment has been minimized.

Contributor

grlee77 commented Jun 2, 2016

another test idea:
upcoming scipy 0.18 will have spherical Voronoi diagram support. That would allow you to verify that each point represents an approximately equal area on the spherical surface.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 3, 2016

@arokem I added two tests, one to verify that we are getting unit vectors and the other to check orthogonality when we generate two vectors.

@grlee77 Thanks. I will keep an eye on the latest update and add tests accordingly.

Shahnawaz Ahmed

@sahmed95 sahmed95 force-pushed the sahmed95:generate_bvec branch from 316d5db to 7c67f05 Jun 3, 2016

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Jun 3, 2016

Simple but useful! Thanks @sahmed95! For my side, this can be merged!

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Jun 3, 2016

Well It can be merged after resolving the issue with numpy 1.7.1 compatibility =P

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 3, 2016

:-( I am using numpy 1.9.1 and its passing on my system since linalg.norm accepts the axis parameter. I see that Travis (current dipy?) uses version 1.7.1. I can simply calculate the norm without using numpy.

Should I do that or let this wait till dipy updates dependency to 1.9.1 ?

Shahnawaz Ahmed
@arokem

This comment has been minimized.

Member

arokem commented Jun 3, 2016

The reason we test with older versions of numpy is that we want scientists
to be able to use Dipy, even if they are reluctant to upgrade numpy to
newer versions, because their other software is not up to date. If there is
a way to rewrite the code so that it works on numpy 1.7 that would be best.

On Fri, Jun 3, 2016 at 7:12 AM, Shahnawaz Ahmed notifications@github.com
wrote:

:-( I am using numpy 1.9.1 and its passing on my system since linalg.norm
accepts the axis parameter. I see that Travis (current dipy?) uses version
1.7.1. I can simply calculate the norm without using numpy.

Should I do that or let this wait till dipy updates dependency to 1.9.1 ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1067 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAHPNopwGSDXvh_Grd_xdxhFejUClfRFks5qIDZegaJpZM4IsoaF
.

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Jun 3, 2016

Looks like it may be fixed now but a 3D vector norm isn't hard to do yourself either (bvecs[:,0]**2+bvecs[:,1]**2+bvecs[:,2]**2)**0.5

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 3, 2016

@etpeterson Thanks ! I wanted to use linalg.norm as I thought that it might be faster. The current commit uses a list comprehension [np.linalg.norm(vec) for vec in bvecs]

@arokem

This comment has been minimized.

Member

arokem commented Jun 3, 2016

This looks good to me. @Garyfallidis: is this the right place for this function? Do you have any opinions? If we don't hear back, we'll assume you're fine with this, and I will go ahead with the merge.

@arokem arokem merged commit 9b803a5 into nipy:master Jun 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment