Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11.9k
BUG: fix numpy.quantile for integer inputs#30376
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
DerWeh commentedDec 4, 2025
I fail the test deftest_quantile_gh_29003_Fraction(self):r=np.quantile([1,2],q=Fraction(1))assertr==Fraction(2)assertisinstance(r,Fraction)r=np.quantile([1,2],q=Fraction(.5))assertr==Fraction(3,2)assertisinstance(r,Fraction) Introduced in#29105.
This is undocumented behavior, and I am not quite sure in which case you would want that... Documentation only says:
So my claim is, that if you for whatever reason want |
MaanasArora left a comment
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.
Seems to patch much of the issue! Agreed that the docs are a bit inconsistent for floats still, so let's wait for maintainer review.
Not sure about the fraction edge case, not familiar with this code. But who knows if this will fail with other object dtypes - perhaps one could special-case forq or evengamma.
| q=st.floats(min_value=0,max_value=1), | ||
| ) | ||
| @pytest.mark.filterwarnings("ignore::RuntimeWarning") | ||
| deftest_quantile_properties(a,q): |
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.
Should this not be under theTestQuantile class? Also, perhaps a more descriptive name would be better (maybetest_quantile_within_bounds)?
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.
Should this not be under the
TestQuantileclass?
I don't know, but I can put it there. The function doesn't depend on any attributes and I see no application for inheritance, so I thought a free/plain function is more idiomatic than a method.
Also, perhaps a more descriptive name would be better (maybe
test_quantile_within_bounds)?
If it stays like this, I agree. I planned on adding the type checks here (and any other property we agree on) and extending the test to cover allmethods.
Alternatively, we could write multiple short functions, but it seemed more efficient to me, to scan all inputs only once.
| method_props,gtype) | ||
| result_shape=virtual_indexes.shape+ (1,)* (arr.ndim-1) | ||
| gamma=gamma.reshape(result_shape) | ||
| ifarr.dtype.kindin ("i","u"): |
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 this be merged into the if statement above?
numpy/numpy/lib/_function_base_impl.py
Lines 4785 to 4786 inad80c0a
| ifarr.dtype.kindin"iu": | |
| gtype=None |
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.
Yes, you are probably right. But first, we (or I) need to cover all branches. As pointed out above, in the current draft version, the return type still depends on the quantileq.
I hoped to get feedback from the maintainer if I'm on the right track, before putting in all the effort. As soon as we have the correct tests in place, we should indeed try to simplify the code. Thanks for the input.
As mentioned in#29315 (see#29315 (comment)),
numpy.quantilecurrently can overflow for integer inputs in spite of returning a float:This can be fixed by casting integers to floats, before interpolating.
This yields the correct result:
We ensure a consistent behavior by adding a property test:
So far, this is just a quick fix, I haven't check if all paths are correct.
We probably need to parameterize by the additional parameters of
np.quantile.I just wanted to first cross-check if this is the right approach to fix the issue.
At this point, we still incorrectly get:
While the documentation ensures us that we should get
np.float64(0.0).Currently, the return type depends on the value of
q:which is undesired behavior.
I can add the return type to the property test.
We also haven't looked into the second part of the documentation: "If the input contains ... floats smaller than float64, the output data-type is float64."
Another point worth discussing is using
np.int64with values bigger than can be exactly represented bynp.float64(i.e. integers larger than 2⁵³).I am not quite sure, if we might lose accuracy in this case.
On the other hand, it is clearly documented that we cat
np.float64, so we should expect a loss of accuracy for integers outside the representable range.