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

Addition of two DateOffsets #10902

Closed
1pakch opened this issue Aug 25, 2015 · 7 comments
Closed

Addition of two DateOffsets #10902

1pakch opened this issue Aug 25, 2015 · 7 comments
Labels

Comments

@1pakch
Copy link

1pakch commented Aug 25, 2015

It is convenient to be able to add one DateOffset to another one:

from pandas.tseries.offsets import DateOffset as DO
DO(hour=2) + DO(hour=3) # raises ValueError

Currently it is not supported. Note that DateOffsets basically replicates the functionality of relativedeltas from dateutil module which support addition

from dateutil.relativedelta import relativedelta as RD
RD(hour=2) + RD(hour=3) # yields relativedelta(hour=3)
@jreback
Copy link
Contributor

jreback commented Aug 26, 2015

In [17]: Timestamp('20130101') + DO(hour=2) + DO(hour=3)
Out[17]: Timestamp('2013-01-01 03:00:00')

These do not form composite objects. I suppose they could, but not sure if it has a useful case as these can operate directly on Series/Timestamps.

@aickley pls show the utility here.

@jreback jreback added the Frequency DateOffsets label Aug 26, 2015
@1pakch
Copy link
Author

1pakch commented Aug 26, 2015

This would be useful when 1) offsets are defined at a different place from where there are used and 2) when offsets are defined relative to each other. For instance, currently I have some code conceptually similar to

# Parameters of a study
predictors_window_bdays = 3
predictors_window_end = DateOffset(...) # offset from an event
predictors_window_start = predictors_window_start - DateOffset(bdays=predictors_window_bdays)
# response_window_{bdays,start,end} defined similarly

# The code where the real work is done
# events is a DatetimeIndex()
x = predictors.ix[events+predictors_window_end] - predictors.ix[events+predictors_window_start]
y = response.ix[events+response_window_end]  - response.ix[events+response_window_start]
# analyze the relationship between x and y

Now it is not possible to specify predictor_window_end corresponding to, say, 12:00 of a business day preceding the day of the event (in my view the most intuitive syntax for that would be DateOffset(bdays=-1, hour=12) as suggested in #10903). Additionally, it is not possible to define predictor_window_start relative to predictor_window_end using an intuitive syntax described above.

As a workaround, I am currently storing lists of DateOffsets that are being applied to the index one after the other.

@1pakch 1pakch closed this as completed Aug 26, 2015
@1pakch 1pakch reopened this Aug 26, 2015
@jreback
Copy link
Contributor

jreback commented Aug 26, 2015

This is currently not allowed because adding anything to a BusinessDays (except other business days) is not defined without an absolute reference point. E.g. if you had Timestamp('20130101 11:59:59) + BDay(1) + Second(1) it is clear what to do.

But if you have BDay(1) + Second() then these have to be kept (and applied) independently.

So aside from BusinessHours which I think could be added to a BuisinessDay I don't think this is really possible.

I'll leave it open for you to provide a reasonable proof-of-concept.

@jreback jreback added this to the Someday milestone Aug 26, 2015
@jreback jreback modified the milestones: Next Major Release, Someday Aug 26, 2015
@jreback
Copy link
Contributor

jreback commented Aug 26, 2015

For non-business days I suppose these could be combinable as Timedeltas are.

e.g.

Day() + Hour() -> DateOffset(days=1,hours=1)

@1pakch
Copy link
Author

1pakch commented Aug 26, 2015

Ok, I understand that BDays is not a time unit and hence it's very different from Hour, Minute etc. That being said, in principle, DateOffset can have a bdays offset independent from the other components. When such an extended DateOffset is applied to a date the only ambiguity is whether offsetting by business days or usual time units is done first.

Apart from that, combining BDay with absolute time of the day (BDay() + DateOffset(hour=12, minute=1)) is a transparent operation which is very useful when looking at intraday financial time series.

BTW, surprisingly enough, now one can combine BDay and Second

In [33]: delta = BDay() + Second()
In [34]: (pandas.DatetimeIndex(['2015-08-24']) - delta)[0]
Out[34]: Timestamp('2015-08-21 00:00:01')

and the result is different from the direct application

In [35]: (pandas.DatetimeIndex(['2015-08-24']) - BDay() - Second())[0]
Out[35]: Timestamp('2015-08-20 23:59:59')

which is clearly a bug. The same applies to Hour (I checked) and, I guess, all the other units.

@chris-b1
Copy link
Contributor

@aickley It's not so clear to me that's a bug, because offsets are not necessarily associative.

The way addition of a delta on BDay works right now is that the offset remains a BusinessDay - the additional delta is applied at the last step without regard to sign/count on the offset.

E.g:

In [5]: delta = pd.offsets.BDay() + pd.offsets.Hour()
In [6]: delta
Out[6]: <BusinessDay: offset=Timedelta('0 days 01:00:00')>

In [9]: pd.Timestamp('2015-1-1') + delta
Out[9]: Timestamp('2015-01-02 01:00:00')

In [10]: pd.Timestamp('2015-1-1') + 2 * delta
Out[10]: Timestamp('2015-01-05 01:00:00')

In [11]: pd.Timestamp('2015-1-1') - 2 * delta
Out[11]: Timestamp('2014-12-30 01:00:00')

This definition actually makes sense if you think of the delta as an 'anchored' offset - that is, 1:00 on each business day. But it may be less surprising if the sign/count propagated to the additional delta and that maybe could be changed. It is a bit ambiguous / surprising.

In any case, I'm not seeing why you can't just apply multiple offsets to your data? That would avoid any ambiguity.

@mroeschke mroeschke removed this from the Contributions Welcome milestone Oct 13, 2022
@mroeschke
Copy link
Member

Yeah I think this has been disallowed specifically as it raises a TypeError now and due to #10902 (comment) probably not easy to support so closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants