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

BF: removed ftmp variable #534

Merged
merged 7 commits into from Jan 3, 2015

Conversation

Projects
None yet
3 participants
@Garyfallidis
Member

Garyfallidis commented Dec 30, 2014

Removed unreferenced ftmp variable. This is in response to issue #449. The bug had no real effect because EuDX has already very hard constrains on the boundaries. It doesn't process seeds on the boundary voxels because of limitations with the trilinear interpolation being used. Test showing that added. Thx to the tracking team with the new tracking API we have a more flexible API to deal with the boundaries.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 31, 2014

Test failure on travis :)

@@ -400,7 +400,7 @@ def eudx_both_directions(cnp.ndarray[double,ndim=1] seed,\
#check for boundaries
tmp=ps[i]+step_sz*dx[i]
#ftmp=floor(tmp+.5)

This comment has been minimized.

@matthew-brett

matthew-brett Dec 31, 2014

Member

Remove commented out line?

This comment has been minimized.

@Garyfallidis
@@ -119,5 +119,25 @@ def test_eudx_bad_seed():
assert_raises(ValueError, list, eu)
def test_eudx_boundaries():

This comment has been minimized.

@matthew-brett

matthew-brett Dec 31, 2014

Member

Is it easy to explain why this tests the changed code?

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 31, 2014

Member

Will add some more information.

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 31, 2014

Member

Added information in the test.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 31, 2014

@arokem with the current change of this PR I get a failure from a life test. EuDX will have a slightly different output only near the boundaries of the image. Because your test is using a small ROI this difference is visible. And that is why this command fails with:
npt.assert_(np.corrcoef(matlab_weights, life_fit.beta)[0, 1] > 0.68)
I get a value of 0.62 for np.corrcoef(matlab_weights, life_fit.beta)[0, 1] can we reduce the value from 0.68 or we need to rerun you matlab weights?

@arokem

This comment has been minimized.

Member

arokem commented Jan 2, 2015

Hmm. Seems best to rerun the matlab code with these new tracks. Sorry for holding this up - I will take a look tomorrow.

@arokem

This comment has been minimized.

Member

arokem commented Jan 2, 2015

BTW - is this really for 0.8, or are we planning to do this for 0.9? We do intend to have 0.9 come down before the end of February, so it's not ridiculous to plan to solve it for that, instead of right now.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 2, 2015

This is resolving an issue reported for quite some time. Let's fix it now if possible.

@arokem

This comment has been minimized.

Member

arokem commented Jan 2, 2015

OK - I am working on it.

On Fri, Jan 2, 2015 at 12:33 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

This is resolving an issue reported for quite some time. Let's fix it now
if possible.


Reply to this email directly or view it on GitHub
#534 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Jan 2, 2015

OK - I am done working on this, and I will make a PR with the new files
against this branch.

For future reference, the code to create the matlab weights and rmse is
here:

https://gist.github.com/arokem/5c6a28707273144404ab

The following IPython notebook takes the resulting files, compares to dipy,
and saves the .npy files:

https://gist.github.com/arokem/8d132c35ee94cb564a73

On Fri, Jan 2, 2015 at 12:34 PM, Ariel Rokem arokem@gmail.com wrote:

OK - I am working on it.

On Fri, Jan 2, 2015 at 12:33 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

This is resolving an issue reported for quite some time. Let's fix it now
if possible.


Reply to this email directly or view it on GitHub
#534 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 3, 2015

Can someone merge this?

arokem added a commit that referenced this pull request Jan 3, 2015

Merge pull request #534 from Garyfallidis/ftmp_remove
BF: removed ftmp variable

@arokem arokem merged commit 8156c74 into nipy:master Jan 3, 2015

1 check passed

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