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

Fix stats.scoreatpercentile #3186

Merged
merged 4 commits into from Jan 31, 2014
Merged

Conversation

rgommers
Copy link
Member

@rgommers rgommers commented Jan 4, 2014

Fix ndarray input for per parameter and empty input. Closes gh-1897 and gh-2861.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 07bd6eb on rgommers:fix-scoreatpercentile into fd99d3f on scipy:master.

[0.03, 4.03, 8.03],
[3, 7, 11],
[3, 7, 11]])
assert_equal(res2, expected2)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the _equal is always guaranteed.
Floating point calculations could still introduce a noise, but maybe not in this case (1. * x + 0. * y == x) ? IIUC

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, copying test case without thinking. Will change to assert_allclose.

@josef-pkt
Copy link
Member

looks good to me

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7e72d97 on rgommers:fix-scoreatpercentile into fd99d3f on scipy:master.

@juliantaylor
Copy link
Contributor

scoreatpercentile should be implementable in terms of numpy.percentile in numpy 1.9. That supports all the usual numpy features including extended axis.

@rgommers
Copy link
Member Author

rgommers commented Jan 9, 2014

@juliantaylor that should be done as soon as numpy 1.9 is the minimum supported version I think. I don't see a reason to now do

if np.__version__ < '1.9':
    ....
else:
    # use np.percentile
    ....

@juliantaylor
Copy link
Contributor

its significantly faster as it uses partition instead of sort. but it can still wait a while 1.9 isn't even out yet :)

@josef-pkt
Copy link
Member

My general opinion is that we should point users to numpy.percentile as soon as 1.9 is out and tell them it's much faster, and deprecate this function as soon as possible.

(I haven't checked whether there could be anything specific "stats" that would make it worth keeping. maybe nan handling or masked arrays?)

@rgommers
Copy link
Member Author

rgommers commented Jan 9, 2014

Point out that users should use np.percentile as soon as 1.9 is out: definitely. Deprecating this function: only once we require >=1.9. That'll take a few years probably.

@ev-br
Copy link
Member

ev-br commented Jan 9, 2014

What's wrong with wrapping it with a deprecationwarning as soon as 1.9 is out? (given that DeprecationWarning is off by default with py2.7 anyway)

@rgommers
Copy link
Member Author

rgommers commented Jan 9, 2014

We should only generate DeprecationWarnings for things that are fixable by users once they see them. And many of those users can't upgrade numpy easily (or at all, if their numpy is installed by their admin, or they ship an older numpy version with some product, or ...).

In this case there's also no hurry, given that scoreatpercentile is a perfectly fine function.

@josef-pkt
Copy link
Member

I don't think we need to add a DeprecationWarning already, but we can mention it in the docstring so users/developers can start to switch, and benefit from the faster numpy version if they have it, and don't have to change all their code when we actually deprecate.

"This function is dooomed."

@rgommers
Copy link
Member Author

rgommers commented Jan 9, 2014

Agreed, will do.

@rgommers
Copy link
Member Author

docstring updated

@rgommers
Copy link
Member Author

Can we merge this one now?

ev-br added a commit that referenced this pull request Jan 31, 2014
@ev-br ev-br merged commit 578106c into scipy:master Jan 31, 2014
@ev-br
Copy link
Member

ev-br commented Jan 31, 2014

Thanks, merged.

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

Successfully merging this pull request may close these issues.

scoreatpercentile() does not handle empty list inputs (Trac #1372)
5 participants