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

BF: added tolerance for negative streamline coordinates checks #678

Merged
merged 1 commit into from Oct 26, 2015

Conversation

Projects
None yet
3 participants
@jchoude
Contributor

jchoude commented Jul 14, 2015

In some case, tracts can be generated exactly on the border of the image volume. When mapped back to voxel coordinates, due to numerical imprecisions, they may have a coordinate with a value like -5.96046447754e-08, which trips up the negative coordinates test in _to_voxel_coordinates.

I added a tolerance to avoid rejecting valid points. Using a very small threshold so that genuinely invalid points still end up raising the error.

BF: added tolerance for negative streamline coordinates checks
In some case, tracts can be generated exactly on the border of the
image volume. When mapped back to voxel coordinates, due to numerical
imprecisions, they may have a coordinate with a value like
-5.96046447754e-08, which trips up the negative coordinates test
in _to_voxel_coordinates. Added a tolerance to avoid rejecting
valid points.
@arokem

This comment has been minimized.

Member

arokem commented Jul 14, 2015

Would it be possible to provide a test that fails without this fix?

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jul 14, 2015

Of course! Just to make sure, you want a dataset that fails without this, or a new test in the testing framework?

If it's the latter, where should I put it: as a new test function in dipy.tracking.tests.test_utils.py, or as an additional test in, say, test_density_map of the same file?

@arokem

This comment has been minimized.

Member

arokem commented Jul 14, 2015

The latter!

As for your question, that depends: ideally as many atomic tests as possible. On the other hand, if your test would need to use variables that are already defined in this test, it might be more expedient to just add this as a test-case in there.

@@ -60,7 +60,7 @@ def _to_voxel_coordinates(streamline, lin_T, offset):
raises an error for negative voxel values."""
inds = np.dot(streamline, lin_T)
inds += offset
if inds.min() < 0:
if inds.min().round(decimals=6) < 0:
raise IndexError('streamline has points that map to negative voxel'

This comment has been minimized.

@Garyfallidis

Garyfallidis Sep 14, 2015

Member

Start with a capital letter please.

This comment has been minimized.

@jchoude

jchoude Oct 23, 2015

Contributor

@Garyfallidis just to make sure, you mean to start the error message string? I'll fix it, was already like this before.

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 26, 2015

Member

Ignore this. Not important.

This comment has been minimized.

@arokem

arokem Oct 26, 2015

Member

Oh yeah - this PR is also crucial for the release. We need a test!

On Mon, Oct 26, 2015 at 7:28 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/tracking/_utils.py
#678 (comment):

@@ -60,7 +60,7 @@ def _to_voxel_coordinates(streamline, lin_T, offset):
raises an error for negative voxel values."""
inds = np.dot(streamline, lin_T)
inds += offset

  • if inds.min() < 0:
  • if inds.min().round(decimals=6) < 0:
    raise IndexError('streamline has points that map to negative voxel'

Ignore this. Not important.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/678/files#r42999436.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 14, 2015

@jchoude add a quick testing grabbing the IndexError and will merge this asap. Thank you very much! 💃

@arokem

This comment has been minimized.

Member

arokem commented Oct 5, 2015

@jchoude - any chance to revive this PR? A small test, pretty please?

@jchoude

This comment has been minimized.

Contributor

jchoude commented Oct 6, 2015

@arokem I haven't forgotten, just got swamped as always. Will try to do in the next few days.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 18, 2015

Ping! Because this problem breaks execution we need this merged before the release. Sorry for the rush. Let us know what you think. And if it is possible to have the test added soon so we can merge this.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2015

Okay @jchoude because this is an important fix. I will go ahead and merge this PR. Can you write a test as @arokem suggests in a new PR?

Garyfallidis added a commit that referenced this pull request Oct 26, 2015

Merge pull request #678 from jchoude/BF_add_tolerance_for_negative_co…
…ordinates

BF: added tolerance for negative streamline coordinates checks

@Garyfallidis Garyfallidis merged commit 5728984 into nipy:master Oct 26, 2015

1 check passed

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

This comment has been minimized.

Member

arokem commented Oct 26, 2015

I didn't mean that you should merge this without a test!

Now we're almost guaranteed that this will remain untested - sure way of
getting the bug back in...

On Mon, Oct 26, 2015 at 7:34 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Merged #678 #678.


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

@jchoude

This comment has been minimized.

Contributor

jchoude commented Oct 26, 2015

Damn... I was just finishing the test :( I have it ready here... Will do the new PR then.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2015

👯 now I got your attention 👯 🎱

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2015

:)

@jchoude

This comment has been minimized.

Contributor

jchoude commented Oct 26, 2015

you already had my attention... we're in the same lab, you could have pigned me.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2015

Yeah, sorry. No worries. 👍

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