8000 MAINT: weightedtau, change search for nan by andyfaff · Pull Request #8282 · scipy/scipy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: weightedtau, change search for nan #8282

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 13, 2018
Merged

MAINT: weightedtau, change search for nan #8282

merged 2 commits into from
Jan 13, 2018

Conversation

andyfaff
Copy link
Contributor

Appveyor is currently failing due to an issue with weightedtau. There's a runtime warning from np.min if the array contains a NaN. However, I'm having problems reproducing the warning in a repeatable fashion. I suspect it's probably due to something changing in numpy 1.14. I'm asking on the numpy ML what this could be.

The changes in this PR should fix the test breakage, I would even hazard to say that it's more pythonic than the original.

josef-pkt
josef-pkt reviewed Jan 11, 2018
View reviewed changes
scipy/stats/stats.py Outdated
y = _toint64(y)
if np.isnan(x).any():
x = _toint64(x)
if np.isnan(y).any():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember seeing a stackoverflow question where np.isnan(np.ptp(y)) is faster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird behaviour with np.min is what leads us to this point.

@andyfaff andyfaff mentioned this pull request Jan 11, 2018
Closed
@andyfaff
Copy link
Contributor Author

The np.min warning (on an array that contains np.nan) that leads to the test failure in weightedtau on appveyor seems to depend on compiler settings, etc.
AFAICT this CI failure is coincident with the release of numpy 1.14.
The fix suggested in this PR seems to be as performant as the original code, at least for arrays up to ~10000 in length. With larger arrays it's slower.

An alternate fix is to use the following in the test:

with np.errstate(invalid='ignore'):
     # test

I'm not sure what's preferred.

@andyfaff
Copy link
Contributor Author

np.isnan(np.sum(arr)) can also do the job (and doesn't use a temp array):

>>> np.sum([1,2,3.])
6.0
>>> np.sum([1,np.inf,3.])
inf
>>> np.sum([np.nan,np.inf,3.])
nan

@andyfaff
Copy link
Contributor Author

It's faster still to do:

np.isnan(np.dot(arr, arr))

@andyfaff
Copy link
Contributor Author

Appveyor is working, as is Travis. Looks mergeable?

@rgommers rgommers added the maintenance Items related to regular maintenance tasks label Jan 13, 2018
@rgommers rgommers added this to the 1.1.0 milestone Jan 13, 2018
@rgommers
Copy link
Member

This change looks fine to me, thanks @andyfaff

@rgommers rgommers merged commit 1d5807c into scipy:master Jan 13, 2018
@andyfaff andyfaff deleted the weightedtau branch January 13, 2018 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josef-pkt josef-pkt josef-pkt left review comments

Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
4388
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@andyfaff @rgommers @josef-pkt
0