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

@sinkpoint's power map - refactored #724

Merged
merged 21 commits into from Oct 19, 2015

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Oct 2, 2015

Hi - here are a few suggestions on top of @sinkpoint's implementation of the Dell'Acqua power map (#672). I think that this would be very useful to have, but I have proposed a few changes relative to @sinkpoint's previous implementation that simplify the code and potentially lead to (marginally) faster performance. See here for a diff relative to #672 (ignore the top of this diff in denspeed - that has to do with the fact that #672 needs a rebase):

https://github.com/sinkpoint/dipy/pull/1/files

@arokem arokem force-pushed the arokem:sinkpoint-power_map branch 3 times, most recently from a8b134c to bb6a4e3 Oct 13, 2015

@arokem arokem force-pushed the arokem:sinkpoint-power_map branch from 36866c9 to e269414 Oct 17, 2015

@arokem

This comment has been minimized.

Member

arokem commented Oct 19, 2015

OK - here's what I've come up with: I am now testing this with a very simplified set of coefficients, that are all equal to 1. In this case, it is easy to calculate what AP should be equal to: it's the log of the number of even coefficients (because in this case the 2l+1 factors cancel out). Then, the normalization factor has to be taken into account, which I do here by varying that between two different quantities.

This seems like a better solution to me, because it tests the equation, rather than testing some other implementation of the same equation, on a specific test-set. I wouldn't mind having both in here, but I think this is sufficient.

def calculate_max_order(n_coeffs):
"""
Calculate the maximal harmonic order, given that you know the

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 19, 2015

Member

push one line up

for n_coeffs in [6, 15, 28, 45, 66, 91]:
for norm_factor in [0.0005, 0.00001]:
# Create some really simple case:

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 19, 2015

Member

typo: cases

def test_calculate_max_order():
"""
Based on the table in:

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 19, 2015

Member

push one line up please

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 19, 2015

Good job! 👍 Many thanks to @sinkpoint, @arokem and @deflavio!

Garyfallidis added a commit that referenced this pull request Oct 19, 2015

@Garyfallidis Garyfallidis merged commit 2406b05 into nipy:master Oct 19, 2015

1 check was pending

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