-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: Do not claim input to binops is self
(array object)
#18593
Conversation
On the C-side binops only guarantee that one of the operands is an instance of `PyArrayObject *`, but we do not know which one. Typing the first as `PyArrayObject *` is just misleading for almost no actual payoff
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 OK. Just one small nit. I wonder what made you propose this, were there warnings or other indications that this is worth the effort?
@@ -1211,7 +1211,7 @@ PyArray_Conjugate(PyArrayObject *self, PyArrayObject *out) | |||
n_ops.conjugate); | |||
} | |||
else { | |||
return PyArray_GenericBinaryFunction(self, | |||
return PyArray_GenericBinaryFunction((PyObject *)self, |
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.
We never test this function with an out
argument?
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.
Strange... I added a small test, this isn't even documented and positional only (that arr.conjugate(out)
works).
.nb_rshift = array_right_shift, | ||
.nb_and = array_bitwise_and, | ||
.nb_xor = array_bitwise_xor, | ||
.nb_or = array_bitwise_or, |
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.
Was removing the cast here the motivation for this PR?
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.
This was just a side effect. The reason for this was that I got a bit confused by it when I was trying how to add special handling for Python scalars (for the discussion about making np.array(1, dtype=uint8) + 100
return uint8
no matter the value).
(Since there you have to check for both the first and second argument being a Python scalar)
Co-authored-by: Matti Picus <matti.picus@gmail.com>
This can currently only be passed positionally, and this is undocumented.
Thanks @seberg |
On the C-side binops only guarantee that one of the operands
is an instance of
PyArrayObject *
, but we do not know whichone. Typing the first as
PyArrayObject *
is just misleadingfor almost no actual payoff
I don't care much about this, one if you prefer the old version, happy to just close.