Skip to content
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

Merged
merged 17 commits into from Aug 1, 2019

Conversation

@frheault
Copy link
Contributor

commented Jul 30, 2019

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.

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 30, 2019

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
@skoudoro
Copy link
Member

left a comment

Thank you for this update @frheault. Below a couple of comments. I still need to run everything.

self.affine,
seeds=seeds)

return utils.transform_tracking_output(track, self.affine,

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

What is this function? Why not transform_streamlines from dipy.tracking.streamlines?

This comment has been minimized.

Copy link
@frheault

frheault Jul 30, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 31, 2019

Member

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)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

Why do you remove this ?

This comment has been minimized.

Copy link
@frheault

frheault Jul 30, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

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():

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

I do not understand this Condition

This comment has been minimized.

Copy link
@frheault

frheault Jul 30, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 31, 2019

Member

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():

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

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):

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 30, 2019

Member

I think this function should be deleted in favor of transform_streamlines. You can add the seed management inside transform_streamlines

This comment has been minimized.

Copy link
@frheault

frheault Jul 30, 2019

Author Contributor

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))

This comment has been minimized.

Copy link
@Garyfallidis

Garyfallidis Jul 30, 2019

Member

this is already returning a list

@skoudoro skoudoro added this to the 1.0 milestone Jul 31, 2019

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))

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 31, 2019

Member

you should remove list since transform_streamlines return a list and not an iterator like move_streamlines

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Can you address my small comment above @frheault, and then this PR is ready to go!

@codecov

This comment has been minimized.

Copy link

commented Jul 31, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@637e5e7). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1939   +/-   ##
========================================
  Coverage          ?   84.6%           
========================================
  Files             ?     119           
  Lines             ?   14653           
  Branches          ?    2325           
========================================
  Hits              ?   12397           
  Misses            ?    1714           
  Partials          ?     542
Impacted Files Coverage Δ
dipy/tracking/streamline.py 92.01% <100%> (ø)
dipy/tracking/utils.py 92.77% <100%> (ø)
dipy/workflows/tracking.py 96.51% <100%> (ø)
dipy/tracking/local/localtracking.py 95.78% <100%> (ø)
@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Hi @frheault, I just merge #1926(local tracking) so this PR needs a rebase. Can you do it? Thank you!

@skoudoro skoudoro merged commit 64641b2 into nipy:master Aug 1, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Thank you @frheault

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.