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/API: Fix operating with timedelta64/pd.offsets on rhs of a datelike series/index #4534

Merged
merged 4 commits into from
Aug 13, 2013

Conversation

jreback
Copy link
Contributor

@jreback jreback commented Aug 10, 2013

closes #4532
closes #4134
closes #4135
closes #4521

Timedeltas can be converted to other ‘frequencies’ by dividing by another timedelta.

In [210]: td = Series(date_range('20130101',periods=4))-Series(date_range('20121201',periods=4))

In [211]: td[2] += np.timedelta64(timedelta(minutes=5,seconds=3))

In [212]: td[3] = np.nan

In [213]: td

0   31 days, 00:00:00
1   31 days, 00:00:00
2   31 days, 00:05:03
3                 NaT
dtype: timedelta64[ns]

to days

In [214]: td / np.timedelta64(1,'D')

0    31.000000
1    31.000000
2    31.003507
3          NaN
dtype: float64

to seconds

In [215]: td / np.timedelta64(1,'s')

0    2678400
1    2678400
2    2678703
3        NaN
dtype: float64

Dividing or multiplying a timedelta64[ns] Series by an integer or integer Series yields a float64 dtyped Series.

In [216]: td * -1

0   -31 days, 00:00:00
1   -31 days, 00:00:00
2   -31 days, 00:05:03
3                  NaT
dtype: timedelta64[ns]

In [217]: td * Series([1,2,3,4])

0   31 days, 00:00:00
1   62 days, 00:00:00
2   93 days, 00:15:09
3                 NaT
dtype: timedelta64[ns]

…ta64 not working the same

TST: add in @cpcloud tests related to GH4135
TST: add tests/catch for non-absolute DateOffsets in timedelta operations
@jreback
Copy link
Contributor Author

jreback commented Aug 13, 2013

@cpcloud any more thoughts on this?

was thinking all of the timedelta/date arithmetic out of core/series.py, maybe to core/ops.py? (just to make code cleaner)

cc @jtratner, IIRC you are creating an core/ops.py for something?

@jreback
Copy link
Contributor Author

jreback commented Aug 13, 2013

bombs away

jreback added a commit that referenced this pull request Aug 13, 2013
BUG/API: Fix operating with timedelta64/pd.offsets on rhs of a datelike series/index
@jreback jreback merged commit 9fc8636 into pandas-dev:master Aug 13, 2013
@cpcloud
Copy link
Member

cpcloud commented Aug 13, 2013

👍

@jtratner
Copy link
Contributor

@jreback which of these should be allowed with rops? E.g., clearly okay to do __radd__ and __rmul__ since those are transitive. Should rdiv work? What about rsub?

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

I think the reversals are somewhat handled (e.g. lhs of int and rhs of td is ok when doing mul)
radd ok always I think, rsub I think is never ok (except td and td is ok, but datetime and td are NOT ok)
div I think order IS important (e.g. integer and td is not allowed, but td and integer is)

@jtratner
Copy link
Contributor

@jreback so the problem is that, with how the arithmetic functions (and r* funcs in general) are set up:

t1 - t2
t2 - t1

get the same call signature for self, other in wrapper, and you only know what's going on because of the name variable that's passed along. So, would it work to just do:

# earlier
if r_op:
    external_op = op
    op = lambda x, y: external_op(y, x)
if r_op:
   rvalues, lvalues = self, other
else:
   lvalues, rvalues = self, other

and then flip the ops when they actually get passed to the operation (since all r ops can't currently be passed well through core.expressions.evaluate). This ends up with the following chain of operations [specific to series, because it's the only one that actually cares about left and right values]:

#r_op passed into arith_method
r_op = lambda x, y: operator.add(y, x)
internal_op = lambda a, b: r_op(b, a)
# inside method
rvalues, lvalues = self, other
# finally at end, gets called
internal_op(lvalues, rvalues) --> internal_op(other, self) --> r_op(self, other) --> operator.add(other, self)

That should make all the checks work for it, right?

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

can we add a flag to the wrapper to indicate if its a reversed arg op? (eg. True for radd and False for add)?
then easy enough toreflip them at the beginning to make the order correct?

@jtratner
Copy link
Contributor

@jreback yeah, that's what I outlined. There's already a flag (since it gets the name), it's just:

r_op = name[0] == "r" or name.startswith("__r")

and it happens when the method is bound, not when it's called, so the cost of the check is minimal.

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

@jtratner i c now....getting late!

I think if you see a reversed op, do:

self, other = other, self

because the timedelta/datetime compuations are very sensitive to the type of the lhs operator (even before values are calculated), e.g. lhs=td, rhs=integer -> radd -> lhs=integer, rhs=td will blow up as don't know how to deal wit this, its required to have the td on the lhs (I mean it could deal with it but just easier this way)

@jtratner
Copy link
Contributor

@jreback can't do that, because the other could be a scalar or something, right?

@jtratner
Copy link
Contributor

@jreback figured it out, it's all good

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

great!

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

aren't the r??? methods ONLY called if the regular methods don't have a valid method, e.g.

td_series + integer -> ok we call add
integer + td_series -> call radd?

@jtratner
Copy link
Contributor

@jreback yes, but the call signature is the same both times

integer + td_series --> td_series.__radd__(integer) --> wrapper(td_series, integer)
td_series + integer --> td_series.__add__(integer) --> wrapper(td_series, integer)

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

ahh...so that's ok (in this case then), but __name__ is radd?

@jtratner
Copy link
Contributor

yes (because name is used to set __name__ of the wrapper method, so it's
already in the function's lexical scope).

@jreback
Copy link
Contributor Author

jreback commented Aug 15, 2013

gotcha

FYI

minor rebasing to master of series2, (incorporates json changes) and some doc cleanups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment