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

Week.onOffset looks fishy #18510

Closed
jbrockmendel opened this Issue Nov 27, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@jbrockmendel
Member

jbrockmendel commented Nov 27, 2017

A Week offset with weekday=None behaves very much like a Tick offset. The main difference is how they handle onOffset. Assuming normalize is False, the Week object will always return week.onOffset(other) --> False while the Day object will always return day.onOffset(other) --> True.

Is this intentional?

@gfyoung

This comment has been minimized.

Member

gfyoung commented Nov 27, 2017

@jbrockmendel : Would you mind posting a small code sample for this just for reference?

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Nov 27, 2017

In [2]: week = pd.offsets.Week()
In [3]: day7 = pd.offsets.Day(n=7)
In [5]: ts = pd.Timestamp.now()
In [6]: ts
Out[6]: Timestamp('2017-11-27 13:03:35.592386')
In [7]: ts + day7
Out[7]: Timestamp('2017-12-04 13:03:35.592386')
In [8]: ts + week
Out[8]: Timestamp('2017-12-04 13:03:35.592386')
In [9]: week.onOffset(ts)
Out[9]: False
In [10]: day7.onOffset(ts)
Out[10]: True
In [12]: day7.rollforward(ts)
Out[12]: Timestamp('2017-11-27 13:03:35.592386')
In [13]: week.rollforward(ts)
Out[13]: Timestamp('2017-12-04 13:03:35.592386')

It isn't that the Week.onOffset behavior is wrong per se, just that I don't see why it behaves this way. In every other way it behaves like a Tick offset.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Nov 27, 2017

@jbrockmendel : Indeed, it's difficult for me to say that it's wrong when there is ZERO documentation on that function. 😄 (in fact, if you would like to add documentation to that method, be my guest!)

That being said, I do see what you mean re: consistency. @jreback @jorisvandenbossche thoughts?

@gfyoung gfyoung added the Docs label Nov 27, 2017

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Dec 13, 2017

@jreback is it the case that offset.onOffset(dt) should always be the same as (the base class implementation) (dt + offset) - offset == dt? If so, this is a bug and should be resolved before #18738.

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Dec 14, 2017

@gfyoung mind changing the tag from Timedelta to Frequencies?

@gfyoung gfyoung added Frequency and removed Timedelta labels Dec 14, 2017

@gfyoung

This comment has been minimized.

Member

gfyoung commented Dec 14, 2017

@jbrockmendel : done

@jreback jreback added this to the 0.23.0 milestone Dec 21, 2017

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