_streamlines_in_mask bounds check #2183

takhs91 · 2020-06-12T23:16:34Z

Adds bound checking in _streamlines_in_mask when accessing the mask array.

I also changed it to explicitly return 1 or 0 for the final point to avoid returning unwanted values given in the mask array

arokem · 2020-06-15T21:52:32Z

Looks legit to me. Could you maybe add a test case along the lines of what you had in #2182 ?

takhs91 · 2020-06-16T17:05:30Z

I have updated that based on my comment #2182 (comment)

jchoude · 2020-06-16T19:24:24Z

That's a good check to have as a general case.

However, when calling target_line_based, in #2182 you mention using a mask coming a different anatomical reference and bounding box than the DWI used to generate the streamline. I'm not sure if this should be handled here or not, but this can be quite risky if the affines are not handled correctly, since the basic assumption here is that the streamline points and intersections to the grid are mapped to a grid which is based on the dimensions of the reference anatomy. If the dimensions do not fit with the content of the streamlines file, there's a very probable chance of getting inconsistent results due to wraparound, which would not necessarily be caught here.

takhs91 · 2020-06-17T12:54:21Z

Hi @jchoude,

you mention using a mask coming a different anatomical reference and bounding box than the DWI used to generate the streamline.

I mean using a mask taken on a anatomical e.g. T1 image of the same subject in the same scanner that might have a smaller bounding box than the DWI image. The affine provided in the target_line_based would have to be the appropriate one to bring the streamline points to the masks's coordinate system of course. You might have also cropped the mask array based on the masks bounding box to save space (its affine will had to be updated ).

If the dimensions do not fit with the content of the streamlines file, there's a very probable chance of getting inconsistent results due to wraparound, which would not necessarily be caught here.

This is what I try to fix with this PR. The points reaching _streamline_in_mask might be out of bounds of the mask's shape, the line based algorithm will work on them(since I believe they are handled correctly in the rest of the algorithm except from when accessing the mask array (and when were turned to x,y,z with truncation instead of floor)) but the mask array will never be accessed with out of bounds indices.

In that way you can still pass streamlines that may completely or partially fall out of masks's shape to target_line_based and they will be still filtered correctly. Out of bound points will simply get ignored.

there's a very probable chance of getting inconsistent results due to wraparound, which would not necessarily be caught here.

I think that what was happening was worse than wraparound because when out of bounds indices were passed here for example it was even returning random values other than 0 and 1 that were only included in the mask array. I was even getting segmentation fault at some cases as I state in the issue. I think it was reading out of bounds as it is possible to do in C language and since it is Cython this is possible I guess.

Do you think after the changes in this PR, out of bounds points still create an issue for this function? If so can you please provide me with an example?

takhs91 · 2020-07-02T15:23:37Z

Hi! Any updates on that?

Garyfallidis · 2020-07-02T23:06:54Z

@jchoude can you please suggest further steps ?

jchoude · 2020-07-03T01:51:06Z

In fact, the fix works. I was just wary of people getting edge cases when misusing the internal function.

Code looks good to me.

Just not sure why CICD is not working.

guaje

Overall looks good to me. I just would add these comments.

guaje · 2020-07-03T06:23:06Z

dipy/tracking/vox2track.pyx

+            x = <cnp.npy_intp>floor(current_pt[0] + half_ratio * direction[0])
+            y = <cnp.npy_intp>floor(current_pt[1] + half_ratio * direction[1])
+            z = <cnp.npy_intp>floor(current_pt[2] + half_ratio * direction[2])
+            if x >= 0 and y >= 0 and z >= 0 and x < mask.shape[0] and y < mask.shape[1] and z < mask.shape[2]:


This could be expressed in a more pythonic way as follows:

if 0 <= x < mask.shape[0] and 0 <= y < mask.shape[1] and 0 <= z < mask.shape[2]:

guaje · 2020-07-03T06:29:30Z

dipy/tracking/tests/test_utils.py

+    (np.array([[1, -10, 0], [1, 10, 0]]), 1),
+])
+def test_target_line_based_out_of_bounds(streamline, expected_matched):
+    # Ensures https://github.com/dipy/dipy/issues/2182 doesn't happen


Could you please remove this comment?

guaje · 2020-07-03T06:55:39Z

dipy/tracking/tests/test_utils.py

@@ -313,6 +314,24 @@ def _target(target_f, streamlines, voxel_both_true, voxel_one_true,
    assert_true(exclude[0] is streamlines[1])


+@pytest.mark.parametrize("streamline, expected_matched", [


As you can see, the rest of the test cases don't utilize this way to pass parameters, mainly because we don't want to have pytest as a dependency. Therefore, could you please change this and remove the import.

guaje

@takhs91 thanks for the update. LGTM!

skoudoro · 2020-08-17T18:50:16Z

Hi @takhs91,

Can you rebase your PR to be sure that everything is ok? Thank you.

After the rebase and If everything is ok, we will go ahead and merge it. Thank you

…asks's bounds

codecov · 2020-08-18T10:59:23Z

Codecov Report

Merging #2183 into master will increase coverage by 2.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2183      +/-   ##
==========================================
+ Coverage   89.37%   91.37%   +2.00%     
==========================================
  Files         251      251              
  Lines       32507    32514       +7     
  Branches     3422     3423       +1     
==========================================
+ Hits        29052    29709     +657     
+ Misses       2736     2057     -679     
- Partials      719      748      +29

Impacted Files	Coverage Δ
dipy/tracking/tests/test_utils.py	`98.15% <100.00%> (+0.03%)`	⬆️
dipy/io/utils.py	`86.41% <0.00%> (+0.54%)`	⬆️
dipy/workflows/stats.py	`85.81% <0.00%> (+0.70%)`	⬆️
dipy/nn/model.py	`92.59% <0.00%> (+1.85%)`	⬆️
dipy/nn/tests/test_tf.py	`88.46% <0.00%> (+1.92%)`	⬆️
dipy/io/streamline.py	`81.48% <0.00%> (+3.70%)`	⬆️
dipy/io/tests/test_stateful_tractogram.py	`93.20% <0.00%> (+4.07%)`	⬆️
dipy/utils/optpkg.py	`78.26% <0.00%> (+4.34%)`	⬆️
dipy/io/tests/test_streamline.py	`97.70% <0.00%> (+8.04%)`	⬆️
dipy/viz/__init__.py	`65.00% <0.00%> (+30.00%)`	⬆️
... and 6 more

skoudoro · 2020-08-18T16:06:34Z

Thank you very much @takhs91 for the rebase and the update. merging

takhs91 mentioned this pull request Jun 16, 2020

target_line_based might read out of bounds #2182

Closed

takhs91 marked this pull request as ready for review June 16, 2020 18:04

guaje reviewed Jul 3, 2020

View reviewed changes

takhs91 requested a review from guaje July 4, 2020 15:50

guaje approved these changes Jul 6, 2020

View reviewed changes

skoudoro added this to the 1.2 milestone Jul 21, 2020

skoudoro added the type:Enhancement label Jul 21, 2020

takhs91 added 8 commits August 18, 2020 12:53

Add bounds checking before accessing mask array in _streamline_in_mask

c710c00

Add explicit return 0 at the end of _streamline_in_mask

0c83bbc

Use floor to indices before casting to int in _streamline_in_mask

6a612f9

Add test for target_line_based for streamline points falling out of m…

bdcaf90

…asks's bounds

Change test parameter name

dadc8fe

Remove comment

8842fb9

Rewrite comparisons in more pythonic way

1c1c4dc

Rewrite target_line_based out of bounds test without pytest

66da528

takhs91 force-pushed the streamline-in-mask-bounds-check branch from 222b013 to 66da528 Compare August 18, 2020 09:56

skoudoro merged commit ecaf8ae into dipy:master Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

_streamlines_in_mask bounds check #2183

_streamlines_in_mask bounds check #2183

takhs91 commented Jun 12, 2020

arokem commented Jun 15, 2020

takhs91 commented Jun 16, 2020

jchoude commented Jun 16, 2020

takhs91 commented Jun 17, 2020

takhs91 commented Jul 2, 2020

Garyfallidis commented Jul 2, 2020

jchoude commented Jul 3, 2020

guaje left a comment •

edited

guaje Jul 3, 2020

guaje Jul 3, 2020

guaje Jul 3, 2020

guaje left a comment

skoudoro commented Aug 17, 2020

codecov bot commented Aug 18, 2020 •

edited

skoudoro commented Aug 18, 2020

		@@ -313,6 +314,24 @@ def _target(target_f, streamlines, voxel_both_true, voxel_one_true,
		assert_true(exclude[0] is streamlines[1])


		@pytest.mark.parametrize("streamline, expected_matched", [

_streamlines_in_mask bounds check #2183

_streamlines_in_mask bounds check #2183

Conversation

takhs91 commented Jun 12, 2020

arokem commented Jun 15, 2020

takhs91 commented Jun 16, 2020

jchoude commented Jun 16, 2020

takhs91 commented Jun 17, 2020

takhs91 commented Jul 2, 2020

Garyfallidis commented Jul 2, 2020

jchoude commented Jul 3, 2020

guaje left a comment • edited

Choose a reason for hiding this comment

guaje Jul 3, 2020

Choose a reason for hiding this comment

guaje Jul 3, 2020

Choose a reason for hiding this comment

guaje Jul 3, 2020

Choose a reason for hiding this comment

guaje left a comment

Choose a reason for hiding this comment

skoudoro commented Aug 17, 2020

codecov bot commented Aug 18, 2020 • edited

Codecov Report

skoudoro commented Aug 18, 2020

guaje left a comment •

edited

codecov bot commented Aug 18, 2020 •

edited