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 : Clamping of the value of v in winding function #937

Merged
merged 4 commits into from Feb 29, 2016

Conversation

Projects
None yet
3 participants
@GuillaumeTh
Contributor

GuillaumeTh commented Feb 22, 2016

With the float's error, it's possible to have a value of v superior at 1 (e.g 1.00000002). For resolve this, I clamp v between -1 and 1.

BF : Clamping of the value of v in winding function
With the float's error, it's possible to have a value of v superior at 1 (e.g 1.00000002). For resolve this, I clamp v between -1 and 1.
@arokem

This comment has been minimized.

Member

arokem commented Feb 23, 2016

Any chance to get a test-case that fails without this fix?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 24, 2016

arccos is mathematically defined between -1 and 1 .

In [3]: np.arccos(-1. - 1e-8)
/usr/bin/ipython:1: RuntimeWarning: invalid value encountered in arccos
  #! /usr/bin/python
Out[3]: nan

In [4]: np.arccos(1. + 1e-8)
/usr/bin/ipython:1: RuntimeWarning: invalid value encountered in arccos
  #! /usr/bin/python
Out[4]: nan

In [5]: np.arccos(1. )
Out[5]: 0.0

In [6]: np.arccos(-1. )
Out[6]: 3.1415926535897931
@GuillaumeTh

This comment has been minimized.

Contributor

GuillaumeTh commented Feb 24, 2016

@arokem I can send you few trk file where we are this warning. I think we have this warning when the angle between two segments is 0.

@arokem

This comment has been minimized.

Member

arokem commented Feb 24, 2016

@GuillaumeTh : please do share a (hopefully small :-D) trk file that has this issue.

The case you described - would that be the case when v1 == v0? It doesn't always cause this:

In [10]: v0 = [1.0, 0.0, 0.0]

In [11]: v1= [1.0, 0.0, 0.0]

In [12]: v=np.dot(v0,v1)/(np.linalg.norm(v0)*np.linalg.norm(v1))

In [13]: tmp=np.arccos(v)

In [14]: v
Out[14]: 1.0
@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 24, 2016

It's a numerical issue, so if you pick 1,0,0 it's always gonna work , there you go

v0 = [np.sqrt(2), np.sqrt(2),np.sqrt(2)]
v1 = [-np.sqrt(2), -np.sqrt(2), -np.sqrt(2)]
v=np.dot(v0,v1)/(np.linalg.norm(v0)*np.linalg.norm(v1))

In [15]: v
Out[15]: -1.0000000000000002

In [16]: tmp=np.arccos(v)
/usr/bin/ipython:1: RuntimeWarning: invalid value encountered in arccos
  #! /usr/bin/python
@arokem

This comment has been minimized.

Member

arokem commented Feb 24, 2016

Great. I think we've narrowed it down to a test case we can add here. Do
you want to add that as a PR against @GuillaumeTh
https://github.com/GuillaumeTh's branch, @samuelstjean?

On Wed, Feb 24, 2016 at 8:10 AM, Samuel St-Jean notifications@github.com
wrote:

It's a numerical issue, so if you pick 1,0,0 it<s always gonna work ,
there you go

v0 = [np.sqrt(2), np.sqrt(2),np.sqrt(2)]
v1 = [-np.sqrt(2), -np.sqrt(2), -np.sqrt(2)]
v=np.dot(v0,v1)/(np.linalg.norm(v0)*np.linalg.norm(v1))

In [15]: v
Out[15]: -1.0000000000000002

In [16]: tmp=np.arccos(v)/usr/bin/ipython:1: RuntimeWarning: invalid value encountered in arccos
#! /usr/bin/python


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

@arokem

This comment has been minimized.

Member

arokem commented Feb 27, 2016

Thanks for the test-case! If you could make the test PEP8 compliant, that would be great (see this commit to see what that looks like: arokem@23293d5). Other than that, ready to be merged as far as I can tell, unless someone else has objections. @samuelstjean - anything more here?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 29, 2016

Seems to work, or at least it does not without the fix, and it is sensible. So I approve.

@arokem

This comment has been minimized.

Member

arokem commented Feb 29, 2016

Thanks! Merging.

arokem added a commit that referenced this pull request Feb 29, 2016

Merge pull request #937 from GuillaumeTh/metrics_winding
BF : Clamping of the value of v in winding function

@arokem arokem merged commit 059d882 into nipy:master Feb 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment