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

BF: Clip the values before passing to arccos, instead of fixing nans. #1708

Merged
merged 3 commits into from Dec 30, 2018

Conversation

@arokem
Copy link
Member

commented Dec 27, 2018

Fixes #1698.

@arokem arokem referenced this pull request Dec 27, 2018
@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

The failure on older numpy/scipy (https://travis-ci.org/arokem/dipy/jobs/472550111#L3347) is puzzling. Does this mean that clip has changed?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

I do not think so. Maybe some nan values create this weird behavior.

Can you try to return np.nan_to_num(xx)?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

BTW, thank you very much for this fix!

@codecov-io

This comment has been minimized.

Copy link

commented Dec 28, 2018

Codecov Report

Merging #1708 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1708      +/-   ##
==========================================
+ Coverage   84.25%   84.26%   +0.01%     
==========================================
  Files         114      114              
  Lines       13557    13556       -1     
  Branches     2139     2139              
==========================================
+ Hits        11422    11423       +1     
+ Misses       1638     1637       -1     
+ Partials      497      496       -1
Impacted Files Coverage Δ
dipy/core/sphere.py 96.29% <100%> (-0.02%) ⬇️
dipy/testing/__init__.py 72.72% <0%> (-9.1%) ⬇️
dipy/reconst/forecast.py 92.22% <0%> (+2.07%) ⬆️
@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

Thanks for taking a look! Like so?

@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

Ugh. I have not been able to install older numpy and scipy on my laptop to debug this.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

neither do I, on my windows desktop or my Osx laptop... I need to find a Linux machine.

I think, we should start to reconsider our minimal version

@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

When I tried to debug the following function:

def angle(x1, x2):
    import ipdb; ipdb.set_trace()
    xx = np.arccos(np.clip((x1 * x2).sum(axis=0), 0, 1))
    return np.nan_to_num(xx)

I obtain something really weird when you look at the basic

with numpy 1.7.1, scipy=0.9

dipy.core.tests.test_sphere.test_interp_rbf ... > 
    543         import ipdb; ipdb.set_trace()
--> 544         xx = np.arccos(np.clip((x1 * x2).sum(axis=0), 0, 1))
    545         return np.nan_to_num(xx)

ipdb> x1.mean(), x2.mean()
(4.2053902447922596e-17,  4.2053902447922596e-17)
ipdb> te = x1 * x2
ipdb> te.mean()
 -3.1434230112588606e-19
ipdb> te = np.multiply(x1, x2)
ipdb> te.mean()
-3.1434230112588606e-19
ipdb>

with numpy 1.13, scipy 1.0.0

    543         import ipdb;ipdb.set_trace()
--> 544         xx = np.arccos(np.clip((x1 * x2).sum(axis=0), 0, 1))
    545         return np.nan_to_num(xx)

ipdb>  x1.mean(), x2.mean()
(4.2053902447922596e-17, 4.2053902447922596e-17)
ipdb> te = x1 * x2
ipdb> te.mean()
-6.7965902946137528e-20
ipdb> te = np.multiply(x1, x2)
ipdb> te.mean()
-6.7965902946137528e-20

As you can see, x1 and x2 are similar in both environnment but their multiplication give a totally different result... I find it really strange. Any thought @arokem?

@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

I'm going to go out on a limb to say that something has changed with floating point representations somewhere along the way. Puzzling indeed.

xx = np.arccos((x1 * x2).sum(axis=0))
xx[np.isnan(xx)] = 0
return xx
xx = np.arccos(np.clip((x1 * x2).sum(axis=0), 0, 1))

This comment has been minimized.

Copy link
@skoudoro

skoudoro Dec 29, 2018

Member

np.clip should be between -1,1 instead of 0,1. It should work correctly after this change. But there is still something which disturbs me from my previous comment.

@arokem

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

@skoudoro

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

CI happy! merging! Thank you @arokem!

You can start to rebase your PR @jhlegarreta

@skoudoro skoudoro merged commit bae5b4c into nipy:master Dec 30, 2018

4 checks passed

codecov/patch 100% of diff hit (target 84.25%)
Details
codecov/project 84.26% (+0.01%) compared to bd95af7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhlegarreta

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2018

🎆 🎉 thanks both ! Rebased all PRs.

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.