-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Fix ndarray + DataFrame ops #23114
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
Conversation
Hello @jbrockmendel! Thanks for submitting the PR.
|
pandas/core/generic.py
Outdated
len(context[1]) == 2 and context[1][1] is self and | ||
isinstance(context[1][0], np.ndarray)): | ||
# TODO: Can we return NotImplemented earlier? By the time we | ||
# get here we've calculated `result`, which may not be cheap |
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.
@shoyer any idea if there is a more elegant way to do this?
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.
What are you trying to fix here? Generally I recommend avoiding __array_wrap__
if possible... __array_ufunc__
is a much more complete interface. To be honest, I'm not even sure if returning NotImplemented
from __array_wrap__
has well-defined behavior.
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.
What are you trying to fix here?
#22537. In master ndarray[int] + DataFrame[timedelta64[ns]] treats the ndarray as timedelta64 instead of raising TypeError.
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.
I'm not familiar with exactly how pandas implements binary ops, but this doesn't look like the right place to fix this.
The problem is likely in the DataFrame.__radd__
or DataFrame.__array_ufunc__
method (which NumPy calls instead of __radd__
if defined, see here for details and recommendations)
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.
Thanks. Looks like its extra-simple: Series and Index both define __array_priority__
but DataFrame does not.
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.
Wow, I'm kind of surprised we never defined DataFrame.__array_priority__
:)
Codecov Report
@@ Coverage Diff @@
## master #23114 +/- ##
==========================================
+ Coverage 92.13% 92.13% +<.01%
==========================================
Files 170 170
Lines 51073 51074 +1
==========================================
+ Hits 47056 47057 +1
Misses 4017 4017
Continue to review full report at Codecov.
|
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.
can you add a whatsnew note. prob need a small example for this.
lgtm |
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff