-
-
Notifications
You must be signed in to change notification settings - Fork 19.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.
|
jreback
left a comment
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