Skip to content
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

MAINT/BUG? __array_ufunc__ should try a given type only once #11306

Closed
mhvk opened this issue Jun 11, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@mhvk
Copy link
Contributor

commented Jun 11, 2018

In the discussion of __array_function__, it was noted that __array_ufunc__ is called on every operand (if present) even if an operand of the given type has already been called [1]. This seems rather senseless: if it didn't work the first time, it won't work the second time either; it may be worth checking this (though it doesn't actually cost anything in the common case that the first trial just works -- but it may speed up things a little).

[1] https://github.com/numpy/numpy/pull/11303/files#diff-95f51a69624c6765b1ddae13dc0616cbR212

@eric-wieser

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

I think for this to make sense, we ought to stop passing the self argument to __array_ufunc__

@shoyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

@eric-wieser would that mean changing __array_ufunc__ to a class method? Sure, in principle that's an option but it seems like a lot of upheaval for no particularly good reason.

In practice, I would guess that most __array_ufunc__ implementations already treat it basically like a class method and ignore the self argument.

@eric-wieser

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

@shoyer: Up to the implementer to decide if they want a @classmethod or @staticmethod, but yes.

it seems like a lot of upheaval for no particularly good reason.

The reason would be to give existing users warning that they'll no longer get to receive every self argument, and prevent new users from ever building logic that uses self it in the first place.

In principle, we could detect instance methods and emit a warning to keep existing code working.

On the other hand, we clearly declared __array_ufunc__ experimental, so I think we can break it.

@shoyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

I'm not sure how the behavior here is any different here for __array_ufunc__ than it is for builtin Python overloads like __add__/__radd__. If two objects of the same type are being added, you can't wait for __radd__ (where the second argument appears as self) to implement the operation. Python never calls __radd__.

@eric-wieser

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

That's interesting, I did not know that.

@eric-wieser

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

I suppose the difference is that for us, passing self is redundant as the information is already in args - whereas in python, it needs to be passed somehow anyway, so may as well be the first argument.

@shoyer

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

I suppose the difference is that for us, passing self is redundant as the information is already in args - whereas in python, it needs to be passed somehow anyway, so may as well be the first argument.

Yes, it's certainly redundant for us, but on the plus side it makes __array_ufunc__ look more like Python's normal operator special methods. I agree with @mhvk that we shouldn't change now out of some sense of purity.

"Experimental" status for NEPs exists so we can change things that are broken without deprecation cycles. It isn't a pass for us to break things just because we've come up with a slightly better design.

In any case, I'm going to put this issue here into NEP-18, so at least we'll have a clear place to point to when we change this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.