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

BUG: Fix ndarray + DataFrame ops #23114

Merged
merged 10 commits into from Oct 14, 2018

Conversation

Projects
None yet
4 participants
@jbrockmendel
Copy link
Member

commented Oct 12, 2018

  • closes #22537
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@pep8speaks

This comment has been minimized.

Copy link

commented Oct 12, 2018

Hello @jbrockmendel! Thanks for submitting the PR.

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

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 12, 2018

Author Member

@shoyer any idea if there is a more elegant way to do this?

This comment has been minimized.

Copy link
@shoyer

shoyer Oct 12, 2018

Member

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.

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 12, 2018

Author Member

What are you trying to fix here?

#22537. In master ndarray[int] + DataFrame[timedelta64[ns]] treats the ndarray as timedelta64 instead of raising TypeError.

This comment has been minimized.

Copy link
@shoyer

shoyer Oct 12, 2018

Member

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)

This comment has been minimized.

Copy link
@jbrockmendel

jbrockmendel Oct 12, 2018

Author Member

Thanks. Looks like its extra-simple: Series and Index both define __array_priority__ but DataFrame does not.

This comment has been minimized.

Copy link
@shoyer

shoyer Oct 12, 2018

Member

Wow, I'm kind of surprised we never defined DataFrame.__array_priority__ :)

jbrockmendel added some commits Oct 12, 2018

@codecov

This comment has been minimized.

Copy link

commented Oct 13, 2018

Codecov Report

Merging #23114 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23114      +/-   ##
==========================================
+ Coverage   92.13%   92.13%   +<.01%     
==========================================
  Files         170      170              
  Lines       51073    51074       +1     
==========================================
+ Hits        47056    47057       +1     
  Misses       4017     4017
Flag Coverage Δ
#multiple 90.56% <100%> (ø) ⬆️
#single 42.28% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.65% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21e8522...ed7174c. Read the comment docs.

@jreback
Copy link
Contributor

left a comment

can you add a whatsnew note. prob need a small example for this.

@jreback jreback added this to the 0.24.0 milestone Oct 14, 2018

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

lgtm

@jreback jreback merged commit ad2a14f into pandas-dev:master Oct 14, 2018

2 of 6 checks passed

ci/circleci: py27_compat Your tests are queued behind your running builds
Details
ci/circleci: py35_ascii CircleCI is running your tests
Details
ci/circleci: py36_locale CircleCI is running your tests
Details
ci/circleci: py36_locale_slow CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20181014.15 succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

thanks!

@jbrockmendel jbrockmendel deleted the jbrockmendel:arr_with_df branch Oct 23, 2018

brute4s99 added a commit to brute4s99/pandas that referenced this pull request Nov 19, 2018

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.