BUG: Addition raises TypeError if Period is on rhs by sinhrks · Pull Request #13069 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

sinhrks

Period addition raises TypeError if Period is on right hand side.

1 + pd.Period('2011-01', freq='M')
# TypeError: unsupported operand type(s) for +: 'int' and 'pandas._period.Period'

Expected

1 + pd.Period('2011-01', freq='M')
# Period('2011-02', 'M')

@jreback

didn't you make this an issue somewhere? (or maybe just a comment)

jreback

if isinstance(other, (ABCDatetimeIndex, ABCSeries)):
return other + self
elif isinstance(other, Period):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can use ABCPeriod

@sinhrks

No existing issue. Updated the top comment.

jreback

"""
def __radd__(self, other):
# __radd__ on cython extension types like _Period is not used, so

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reference somewhere on this? just curious

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find as long as I searched. We can fix Timestamp if there is a workaround.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on what I mean is where you saw that it didn't work?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once I tried to add __radd__ to cdef Period and found it doesn't work. Then noticed _Timestamp impl. I'm not sure where _Timestamp impl derived from.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh so this means you can leave Period and we don't need _Period. That's only necessary for Timestamp for example because we are extending a c class itself
here the class is already in cython

but what it does mean is that add needs to handle reversed args directly (and similarly for other arith ops)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the doc something like below. This looks work removing _Period class.

   def __add__(self, other):
       if isinstance(self, Period):
           ...
       elif isinstance(other, Period):
           return other + self
       else:
           ...

@codecov-io

Current coverage is 84.14%

Merging #13069 into master will increase coverage by +<.01%

@@ master #13069 diff @@

Files 137 137
Lines 50261 50231 -30
Methods 0 0
Messages 0 0
Branches 0 0

  1. 2 files (not in diff) in pandas/tseries were modified. more
    • Misses -4
    • Hits -22
  2. 2 files (not in diff) in pandas were modified. more
    • Misses -3
    • Hits -5

Powered by Codecov. Last updated by 1296ab3...cdcd3b3

@sinhrks

BUG: Addition raises TypeError if Period is on rhs

@jreback