Skip to content

np.max() doesn't behave properly for ndarray subclasses that define __array_ufunc__ #9102

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
ngoldbaum opened this issue May 11, 2017 · 8 comments
Closed

np.max() doesn't behave properly for ndarray subclasses that define __array_ufunc__ #9102

ngoldbaum opened this issue May 11, 2017 · 8 comments
Labels
00 - Bug component: numpy._core
Milestone
1.13.0 release

Comments

@ngoldbaum
Copy link
Member

ngoldbaum commented May 11, 2017 •
edited
Loading

Take the following example:

import numpy as np

class MyArr(np.ndarray):
    def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
        func = getattr(ufunc, method)
        inp = [i.view(np.ndarray) for i in inputs]
        return func(*inp, **kwargs)

data = np.array([[1, 2, 3], [1, 2, 3]])

arr = data.view(MyArr)

print(data.max())
print(arr.max())

On numpy 1.13rc1 this prints:

3
[1 2 3]

For the call to arr.max() numpy calls __array_ufunc__, but there doesn't seem to be a way to tell from inside __array_ufunc__ that we should be reducing across the whole array and not just the first dimension. I think this would work properly if numpy were calling __array_ufunc__ with axis=None.

Ping @mhvk
@eric-wieser
Copy link
Member

Can you print(inp, kwargs) in there too, and give the output?

@eric-wieser
Copy link
Member

eric-wieser commented May 11, 2017 •
edited
Loading

Actually, nevermind, I see the issue.

@mhvk, your normalization in normalize_reduce_args discards all arguments which are None. This is bad for arguments whose default is not None.

Why do we do this check at all?

@ngoldbaum
Copy link
Member Author

Similarly for np.any:

print(np.any(data))
print(np.any(arr))

prints:

True
[ True  True  True]

Here the issue is that numpy is calling np.logical_or.reduce via umr_any.

@eric-wieser eric-wieser added this to the 1.13.0 release milestone May 11, 2017
@mhvk
Copy link
Contributor

mhvk commented May 11, 2017

Thanks, that is most clearly a bug! I'll try to get a fix in asap!

@mhvk
Copy link
Contributor

mhvk commented May 11, 2017

p.s. The check on None was meant for out -- it is definitely not intended to make this general (it may have something to do with trying to combine all normalizations and then undoing it...)

@mhvk
Copy link
Contributor

mhvk commented May 11, 2017

Actually, no, now that I look at it again, I did this on purpose, mistakenly thinking that None was always the default (and wanting to remove any keyword arguments that were at their default). But in fact for axis, 0 is the default. Anyway, I'll correct this to only remove out (also for accumulate, etc.); I should have realized the code sees the arguments early enough that anything there is explicitly passed in.

@mhvk mhvk mentioned this issue May 11, 2017
Closed
@mhvk
Copy link
Contributor

mhvk commented May 11, 2017

OK, see #9104.

@ngoldbaum - thanks for reporting -- I guess it is likely you'll find more mistakes, but hopefully not more such blatant ones! @eric-wieser - thanks for identifying the bug so quickly.

@mhvk mhvk mentioned this issue May 17, 2017
Merged
@charris
Copy link
Member

charris commented May 17, 2017 •
edited
Loading

See #9106.

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 component: numpy._core
Projects
None yet
Milestone
1.13.0 release
Development

Successfully merging a pull request may close this issue.

BUG: ensure axis=None gets passed on correctly to ufunc.reduce. mhvk/numpy
4 participants
@charris @eric-wieser @mhvk @ngoldbaum