Skip to content

Strange result (and no error/warning) for np.min() on integer array with initial=np.nan #12454

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
Closed
mgeier opened this issue Nov 26, 2018 · 10 comments
Closed

Strange result (and no error/warning) for np.min() on integer array with initial=np.nan #12454

mgeier opened this issue Nov 26, 2018 · 10 comments
Labels
00 - Bug

Comments

@mgeier
Copy link
Contributor

mgeier commented Nov 26, 2018

I know that np.nan is not an integer, but I was still surprised about this behavior:

Reproducing code example:

import numpy as np
print(np.min([1, 2, 3], initial=np.nan))
print(np.max([1, 2, 3], initial=np.nan))
print(np.max([1, 2, 3], initial=9.9))

Result:

-9223372036854775808
3
9

I'm not sure what I was expecting, but somehow I felt that I have to report this ...

Error message:

No error, no warning.

Numpy/Python version information:

1.15.0 3.6.7 (default, Oct 21 2018, 08:08:16)
[GCC 8.2.0]
@seberg
Copy link
Member

seberg commented Nov 27, 2018

I agree, initial was added rather recently. I think we should add a visible Deprecation warning maybe now, and then change the casting to "safe" for it. Just disallowing unsafe casts seems the most straight forward to me.

@eric-wieser
Copy link
Member

@hameerabbasi, want to take a go at this at some point?

@hameerabbasi
Copy link
Contributor

Is the arbitrary object as identity PR already merged? If so, I can change the casting to safe, and it shouldn’t affect anything. If not, we’ll need that to be merged first.

@eric-wieser
Copy link
Member

Yes, that PR is merged - although I'm not sure I see the connection.

@hameerabbasi
Copy link
Contributor

If we changed the casting rule to safe without that, the casting from 1 to True and so on would be forbidden, would it not?

@eric-wieser
Copy link
Member

Oh, maybe. Ultimately I think we're going to want a different identity for each loop type, but that's probably not going to happen any time soon

Can we apply the casting rule to just the initial argument, and let the casting continue to be safe when the identity is used?

@hameerabbasi
Copy link
Contributor

The error seems to be at

numpy/numpy/core/src/umath/reduction.c

Line 489 in 28e5c07

if (PyArray_FillWithScalar(result, identity) < 0) {
, but I have no idea how to change the casting rule to safe inside the PyArray_FillWithScalar function, and I'm not sure we'd want to. If we changed it before and after there would be thread safety concerns.

@hameerabbasi
Copy link
Contributor

We could try casting before and raise an error if a failure occurs.

@hameerabbasi
Copy link
Contributor

I found PyArray_CanCastSafely... But I don't know how to get the PyArray_Descr from the two arrays.

@seberg
Copy link
Member

seberg commented Jun 14, 2022

This now gives a warning at least after gh-21437. That doesn't quite fix the issue for initial=, since it is not clear that initial= (as a ufunc argument!) should allow unsafe casting.

However, we also have the more recent gh-21668 for the same issue, so going to close this one.

@seberg seberg closed this as completed Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@seberg @eric-wieser @mgeier @hameerabbasi