Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
DerWeh wants to merge1 commit intonumpy:main
base:main
Choose a base branch
Loading
fromDerWeh:bugfix/percentile

Conversation

@DerWeh
Copy link
Contributor

As mentioned in#29315 (see#29315 (comment)),numpy.quantile currently can overflow for integer inputs in spite of returning a float:

>>>np.quantile(np.array([32767,-1],dtype=np.int16),0.5)np.float64(49151.0)

This can be fixed by casting integers to floats, before interpolating.
This yields the correct result:

>>>np.quantile(np.array([32767,-1],dtype=np.int16),0.5)np.float64(16383.0)

We ensure a consistent behavior by adding a property test:

quantile=np.quantile(a,q)assertnp.min(a)<=quantileassertquantile<=np.max(a)

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 ofnp.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:

>>>np.quantile([1],0)np.int64(1)

While the documentation ensures us that we should getnp.float64(0.0).
Currently, the return type depends on the value ofq:

>>>np.quantile([1],0.5)np.float64(1.0)

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 usingnp.int64 with 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 catnp.float64, so we should expect a loss of accuracy for integers outside the representable range.

@DerWeh
Copy link
ContributorAuthor

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.
@eendebakpt says:

There are also some corner cases. For example input (of either the array or the quantile) of typeFraction. Possible output types could be eitherFraction orfloat64.

This is undocumented behavior, and I am not quite sure in which case you would want that...

Documentation only says:

If the input contains integers or floats smaller than float64, the output data-type is float64. Otherwise, the output data-type is the same as that of the input. If out is specified, that array is returned instead.

So my claim is, that if you for whatever reason wantFraction as output, you should either explicitly giveout or convert[1, 2] to an array ofFractions.
To my understanding, the test contradicts the documentation.

Copy link
Contributor

@MaanasAroraMaanasArora left a 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):
Copy link
Contributor

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)?

Copy link
ContributorAuthor

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?

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 (maybetest_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"):
Copy link
Contributor

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?

ifarr.dtype.kindin"iu":
gtype=None

Copy link
ContributorAuthor

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@MaanasAroraMaanasAroraMaanasArora left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DerWeh@MaanasArora

[8]ページ先頭

©2009-2025 Movatter.jp