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

Round coords life #818

Merged
merged 3 commits into from Dec 27, 2015

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Dec 24, 2015

Should fix #810

arokem added some commits Dec 24, 2015

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 24, 2015

Combination of PEP8 and fix is hard to follow.

unique_idx = unique_rows(all_coords.astype(int))
else:
unique_idx = unique_idx
unique_idx = unique_rows(np.round(all_coords).astype(np.intp))

This comment has been minimized.

@matthew-brett

matthew-brett Dec 24, 2015

Member

How about leaving the cast to the later line, to avoid users getting errors from passed unique_idx of wrong data type. As in:

        unique_idx = unique_rows(np.round(all_coords))

return _voxel2streamline(transformed_streamline, unique_idx.astype(np.intp))

This comment has been minimized.

@matthew-brett

matthew-brett Dec 24, 2015

Member

Also - need test for modified behavior - such as transformed_streamline with non-integer indices.

@@ -399,11 +398,8 @@ def setup(self, streamline, affine, evals=[0.001, 0, 0], sphere=None):
range_bvecs = np.arange(n_bvecs).astype(int)
# In each voxel:
for v_idx in range(vox_coords.shape[0]):
# dbg:
#if not np.mod(v_idx, 1000):
# print("voxel %s"%(100*float(v_idx)/n_vox))
mat_row_idx = (range_bvecs + v_idx * n_bvecs).astype(np.int32)

This comment has been minimized.

@matthew-brett

matthew-brett Dec 24, 2015

Member

Why int32 here?

This comment has been minimized.

@matthew-brett

matthew-brett Dec 24, 2015

Member

Also - should f_matrix_row, f_matrix_col etc be np.intp?

@arokem

This comment has been minimized.

Member

arokem commented Dec 24, 2015

Sorry about interleaving functional changes together with PEP8 -- the only change (so far) that isn't PEP8 (i.e. white-space) is the rounding and casting to intp. We just otherwise never get to these things....

TST + RF: Add test conditions for fractional streamline coordinates.
Also, fix up rounding in other places: in setting up the model, and in
vox2track.

Had to also reduce the match to the matlab version, but I am not sure
how close we want to stay to that, as we fix up things. At the very
least, the model RMSE is lower here than in the Dipy version, so the
model is a more accurate fit to the signal, for the same number of
parameters.
@arokem

This comment has been minimized.

Member

arokem commented Dec 24, 2015

Added testing, and more calls to round to accommodate the change more widely.

I had to also reduce the match to the matlab version, but I am not sure how close we want to stay to that, as we fix up things (The matlab version uses ceil on the streamline coordinates in these cases: https://github.com/francopestilli/life/blob/master/utility/fefgGet.m#L201, but I am not convinced this is correct).

At the very least, the model RMSE is lower here than in the Matlab version, so the model is a more accurate fit to the signal, for the same number of parameters.

I want to make more changes to LiFE to deal with the memory requirement. I will make a branch off of this work and a separate PR, once we resolve things here.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 24, 2015

Sorry to be dumb - but are the tests such that, if you change the round back to casting to int, they will fail?

@arokem

This comment has been minimized.

Member

arokem commented Dec 24, 2015

Yes - I made sure of that on my laptop, but there's also an explanation:
the float coordinates [1.1, 2.4, 2.9] would be [1, 2, 2] in the
astype(int) case, creating a coordinate that is different and unique from
the first coordinate of the next streamline: [1, 2, 3]. In the round
case, this goes to the same coordinate => [1, 2, 3].

On Thu, Dec 24, 2015 at 3:20 PM, Matthew Brett notifications@github.com
wrote:

Sorry to be dumb - but are the tests such that, if you change the round
back to casting to int, they will fail?


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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 25, 2015

OK - thanks for the explanation - good to go from my PoV.

Garyfallidis added a commit that referenced this pull request Dec 27, 2015

@Garyfallidis Garyfallidis merged commit 7f452b0 into nipy:master Dec 27, 2015

1 check passed

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

This comment has been minimized.

Member

Garyfallidis commented Dec 27, 2015

👍

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