Increase affine consistency in dipy.tracking.utils #1939

frheault · 2019-07-30T12:46:46Z

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.

…ult/dipy into increase_affine_consistency

…ffine_consistency

pep8speaks · 2019-07-30T12:46:56Z

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

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

skoudoro · 2019-07-30T15:27:28Z

dipy/tracking/local/localtracking.py

-                                      self.affine,
-                                      seeds=seeds)
+
+        return utils.transform_tracking_output(track, self.affine,


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

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)

ok, make sense!

skoudoro · 2019-07-30T15:29:35Z

dipy/tracking/tests/test_streamline.py

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


Why do you remove this ?

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

skoudoro · 2019-07-30T15:29:47Z

dipy/tracking/tests/test_streamline.py

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


same question here

skoudoro · 2019-07-30T15:33:38Z

dipy/tracking/utils.py

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


I do not understand this Condition

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)

Thank you for the explanation

skoudoro · 2019-07-30T15:34:00Z

dipy/tracking/utils.py

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


same as above

skoudoro · 2019-07-30T15:37:49Z

dipy/tracking/utils.py

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


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

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

Garyfallidis · 2019-07-30T19:25:49Z

dipy/tracking/tests/test_streamline.py


            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 is already returning a list

skoudoro · 2019-07-31T15:27:32Z

dipy/tracking/tests/test_utils.py

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


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

skoudoro · 2019-07-31T15:33:28Z

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

codecov · 2019-07-31T16:01:55Z

Codecov Report

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

@@           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 · 2019-07-31T19:25:36Z

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

skoudoro · 2019-08-01T02:23:58Z

Thank you @frheault

frheault added 11 commits July 29, 2019 11:17

First pass of deleting/fixing affine usage

a700df9

Fixing all broken function for density, connectivity and seeding

c4e523d

Fix second half of dependacy from utils

aec04cc

Added back move_streamlines under a new name for precision

1046084

First testing phase (test/examples)

7761d5c

Merge branch 'update_file_format_example' of https://github.com/frhea…

f8b6257

…ult/dipy into increase_affine_consistency

Merge with updated examples

4c807b3

Flake8 on everything changed

6fd641a

Fixed invalid reference in tracking workflow

2b8dc6d

Merge branch 'master' of https://github.com/nipy/dipy into increase_a…

b23f155

…ffine_consistency

Updated the api_changes

2706051

frheault added 2 commits July 30, 2019 09:34

Correct typo in docstring, fix example in seeds_from_masks

1ebc15f

Reduce line length

639d922

skoudoro requested changes Jul 30, 2019

View reviewed changes

skoudoro reviewed Jul 30, 2019

View reviewed changes

Added back unsused variable in test, apparently necessary

80c00ec

Garyfallidis reviewed Jul 30, 2019

View reviewed changes

Fixed doctest error

6f846fb

skoudoro added this to the 1.0 milestone Jul 31, 2019

skoudoro reviewed Jul 31, 2019

View reviewed changes

Remove cast for transform_streamlines

7ca62f5

Rebased after PR dipy#1925

bc38dd1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase affine consistency in dipy.tracking.utils #1939

Increase affine consistency in dipy.tracking.utils #1939

frheault commented Jul 30, 2019

pep8speaks commented Jul 30, 2019 •

edited

skoudoro left a comment

skoudoro Jul 30, 2019

frheault Jul 30, 2019

skoudoro Jul 31, 2019

skoudoro Jul 30, 2019

frheault Jul 30, 2019 •

edited

skoudoro Jul 30, 2019

skoudoro Jul 30, 2019 •

edited

frheault Jul 30, 2019

skoudoro Jul 31, 2019

skoudoro Jul 30, 2019

skoudoro Jul 30, 2019

frheault Jul 30, 2019

Garyfallidis Jul 30, 2019

skoudoro Jul 31, 2019

skoudoro commented Jul 31, 2019

codecov bot commented Jul 31, 2019 •

edited

skoudoro commented Jul 31, 2019

skoudoro commented Aug 1, 2019

Increase affine consistency in dipy.tracking.utils #1939

Increase affine consistency in dipy.tracking.utils #1939

Conversation

frheault commented Jul 30, 2019

pep8speaks commented Jul 30, 2019 • edited

Comment last updated at 2019-07-31 19:58:29 UTC

skoudoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frheault Jul 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro Jul 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoudoro commented Jul 31, 2019

codecov bot commented Jul 31, 2019 • edited

Codecov Report

skoudoro commented Jul 31, 2019

skoudoro commented Aug 1, 2019

pep8speaks commented Jul 30, 2019 •

edited

frheault Jul 30, 2019 •

edited

skoudoro Jul 30, 2019 •

edited

codecov bot commented Jul 31, 2019 •

edited