-
Notifications
You must be signed in to change notification settings - Fork 437
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
Increase affine consistency in dipy.tracking.utils #1939
Conversation
…ult/dipy into increase_affine_consistency
…ffine_consistency
Hello @frheault, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2019-07-31 19:58:29 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this update @frheault. Below a couple of comments. I still need to run everything.
dipy/tracking/local/localtracking.py
Outdated
self.affine, | ||
seeds=seeds) | ||
|
||
return utils.transform_tracking_output(track, self.affine, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this function? Why not transform_streamlines
from dipy.tracking.streamlines
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that transform_streamlines should do one thing and one thing only, to stay as simple as possible.
This function takes a generator as a input that can be a tuple or not, and in 99% of the case where people want to transform_streamlines it is just that.
This function with the generator, the tuple and the saving_seeds option is used only once in all dipy and is specifically for the tracking.
I think merging the two function would add to the confusion. And complexify a function that should remain as simple as simple (transform_streamlines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, make sense!
@@ -354,7 +353,6 @@ def test_set_number_of_points_memory_leaks(): | |||
streamlines.append(rng.randn(rng.randint(10, 100), 3).astype(dtype)) | |||
|
|||
list_refcount_before = get_type_refcount()["list"] | |||
rstreamlines = set_number_of_points(streamlines, nb_points=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable, declared variable without details and useless afterward
But I see some of the test crashing (wasn't crashing on my computer)
I will add it back, any idea what is happening
@@ -760,7 +758,6 @@ def test_compress_streamlines_memory_leaks(): | |||
streamlines.append(rng.randn(rng.randint(10, 100), 3).astype(dtype)) | |||
|
|||
list_refcount_before = get_type_refcount()["list"] | |||
cstreamlines = compress_streamlines(streamlines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question here
@@ -397,21 +379,17 @@ def seeds_from_mask(mask, density=[1, 1, 1], voxel_size=None, affine=None): | |||
seeds = seeds.reshape((-1, 3)) | |||
|
|||
# Apply the spatial transform | |||
if affine is not None: | |||
if seeds.any(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand this Condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the affine was optional, so it had to be tested if it existed.
Now it is not need (since it is mandatory)
However I realize that a test supposed to check if empty seed mask were crashing was not actully getting tested because there was no affine provided and so was not getting transform.
So to apply a transform to seed, you need at least one (thats why any() is used, any non zero value in the array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation
@@ -510,7 +490,7 @@ def random_seeds_from_mask(mask, seeds_count=1, seed_count_per_voxel=True, | |||
seeds = seeds[:seeds_count] | |||
|
|||
# Apply the spatial transform | |||
if affine is not None: | |||
if seeds.any(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@@ -919,37 +789,29 @@ def unique_rows(in_array, dtype='f4'): | |||
|
|||
|
|||
@_with_initialize | |||
def move_streamlines(streamlines, output_space, input_space=None, | |||
seeds=None): | |||
def transform_tracking_output(tracking_output, affine, save_seeds=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function should be deleted in favor of transform_streamlines
. You can add the seed management inside transform_streamlines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the other comment, transforming streamlines is confusing enough, I think this would make the transform_streamlines
function less clear by making it doing too many thing at once
|
||
vv = values_from_volume(data, x_sl1, affine=affine) | ||
npt.assert_almost_equal(vv, ans1, decimal=decimal) | ||
|
||
# The generator has already been consumed so needs to be | ||
# regenerated: | ||
x_sl1 = list(ut.move_streamlines(sl1, affine)) | ||
x_sl1 = list(transform_streamlines(sl1, affine)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already returning a list
dipy/tracking/tests/test_utils.py
Outdated
npt.assert_raises(ValueError, list, new) | ||
|
||
# Test smaller voxels | ||
affine = np.array([[.3, 0, 0, 0], | ||
[0, .2, 0, 0], | ||
[0, 0, .4, 0], | ||
[0, 0, 0, 1]]) | ||
streamlines = list(move_streamlines(streamlines, affine)) | ||
new = list(target_f(streamlines, mask, affine=affine)) | ||
streamlines = list(transform_streamlines(streamlines, affine)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should remove list
since transform_streamlines
return a list and not an iterator like move_streamlines
Can you address my small comment above @frheault, and then this PR is ready to go! |
Codecov Report
@@ Coverage Diff @@
## master #1939 +/- ##
========================================
Coverage ? 84.6%
========================================
Files ? 119
Lines ? 14653
Branches ? 2325
========================================
Hits ? 12397
Misses ? 1714
Partials ? 542
|
Thank you @frheault |
To increase consistency within Dipy and increase cautious behaviors when coding the affine is now required when it was previously optional.
Older/unused functions that had complex behavior related to spatial transform and trackvis file format were deleted to prevent confusion.
Docstring is now uniform when describing the affine.
Tests were executed and examples tested.