-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
ENH: Added iqr function to compute IQR metric in scipy/stats/stats.py #5808
Conversation
`scipy.stats.iqr` function | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
Computes the interquatrile region of a distribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "interquartile"
7285db0
to
c3703c2
Compare
@rgommers I have addressed all of your comments. Additionally, I wrapped all the docs to 72 characters except a couple of lines where that did not seem appropriate, and removed the |
8b206da
to
1dbb236
Compare
I am trying to think of how to handle older versions of numpy. Would a warning that "interpolation will be ignored as it is not supported for numpy versions < 1.09" be acceptable/sufficient? |
e21fc2b
to
20357f5
Compare
52dc30d
to
5e8a871
Compare
5e8a871
to
4c72db0
Compare
This PR is aesthetically scary but I hope it can be merged. I'm not quite brave enough to merge this one unilaterally. |
try: | ||
result = np.percentile(x, q, axis=axis, keepdims=keepdims, | ||
interpolation=interpolation) | ||
except TypeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment on the circumstances under which this will happen? It will help the next dev understand what can give rise to this and what needs to be modified.
Other than my nitpicks LGTM. @argriffing I see what you mean about the aesthetics, the |
Thanks for looking at this, @Eric89GXL. I'm OK with this being merged once your comments have been addressed. |
@madphysicist ping |
Sorry for the delay. I'm just getting back to work after having kid #2. I will get to addressing the issues pretty soon. The PR is not dead and I have not forgotten about it. |
Congrats @madphysicist! |
Added tests. The tests run through most of the aspects tested by `numpy.percentile`, from which they are originally derived. Restrictions apply to the functionality as the version of numpy decreases below 1.11.0. The tests and documentation make this explicit.
4c72db0
to
49d9b7c
Compare
I have addressed all the comments via appropriate code changes. While I personally have a preference for using The aesthetics of this function are a good indication of the fact that it belongs in numpy rather than here. Perhaps if I ever get around to implementing my weighted percentile function, that will be enough motivation to allow the move. |
Needs a rebase |
All right, it's been okayed by Alex and Eric, so I rebased and merged it in 8346eea |
Congratulations @madphysicist ! |
Transfer of numpy PR numpy/numpy#7137.
This function is different from the IQR in statsmodels. It appears to have different inputs (one array versus two), which actually makes this version more general.
As per @rgommers's request, I have added
nan_policy
, which basically just selects betweennp.percentile
andnp.nanpercentile
. Since there were some bugs in the shape of arrays returned bynp.nanpercentile
, I have added a workaround that should eventually be removed.As per @josef-pkt's request, I have added a 'scale' argument that accepts the values
'raw'
or1.0
,'normal'
orsp.special.erfinv(0.5) * 2 * sp.sqrt(2) ~= 1.349
, or any other number. The result is divided by the scale. The default is'raw'
.The tests have been transferred almost as-is from numpy. However, if the numpy version being used is < 1.11.0, tests involving
interpolation='midpoint'
are turned off because bug-fix numpy/numpy#7129 and its backport numpy/numpy#7166 would not be in effect.