-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Timestamp origin takes no effect in resample for 'MS' frequency #53938
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
Conversation
It seems the The change adds a simple check to modify the way the range is set This gives the behaviour you're after @MarcoGorelli and preserves all other functionality. |
thanks for your pr - this looks a bit complex, but it's encouraging that tests pass - I'll check next week! |
Hey @MarcoGorelli just bumping this up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR
Does this always work? For example, if we have
In [39]: df = pd.DataFrame({'ts': [datetime(1999, 12, 31, 20)], 'values': [10.]})
In [40]: df
Out[40]:
ts values
0 1999-12-31 20:00:00 10.0
and do
df.resample('3YS', on='ts', closed='left', label='left', origin=datetime(1995, 1, 1))['values'].sum()
then I'd expect the windows to be:
- [1995-01-01, 1998-01-01)
- [1998-01-01, 2001-01-01)
But instead, I see:
In [42]: df.resample('3YS', on='ts', closed='left', label='left', origin=datetime(1995, 1, 1))['values'].sum()
Out[42]:
ts
1999-01-01 10.0
Freq: 3AS-JAN, Name: values, dtype: float64
I'll need to switch approaches. My solution only covers cases where the origin is within the period specified. |
could always raise an error if it's not, we don't need to do 4-D gymnastics, the most important thing is that for the cases when the computation succeeds, it's correct |
Alright @MarcoGorelli I updated what I was doing to be a bit more straightforward. |
Looks like I also need to update the Docs with the new expected behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tz-naive, this seems correct. Nice one!
I think we just need some extra care for the tz-aware case
Added a check on the equality of Daylight Savings Time via |
thanks for updating - this looks close 💪 |
Anything else you want to see here @MarcoGorelli? |
this is probably fine - will take a (hopefully final) look tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, great work here
Leaving open a little in case anyone has objections, but I think this should go in for 2.1
…andas-dev#53938) * add missing origin check * whats new * resolve edge cases, fix tests * update docs * cleanup * accomidate daylight savings time * correct docs and remove nested checks * slim down example * add back tz aware test
…andas-dev#53938) (#17) * add missing origin check * whats new * resolve edge cases, fix tests * update docs * cleanup * accomidate daylight savings time * correct docs and remove nested checks * slim down example * add back tz aware test Co-authored-by: Conrad Mcgee Stocks <[email protected]>
…andas-dev#53938) * add missing origin check * whats new * resolve edge cases, fix tests * update docs * cleanup * accomidate daylight savings time * correct docs and remove nested checks * slim down example * add back tz aware test
…andas-dev#53938) (#29) * add missing origin check * whats new * resolve edge cases, fix tests * update docs * cleanup * accomidate daylight savings time * correct docs and remove nested checks * slim down example * add back tz aware test Co-authored-by: Conrad Mcgee Stocks <[email protected]>
…equency (pandas-dev#53938)" This reverts commit dfc4c39.
…equency (pandas-dev#53938)" This reverts commit dfc4c39.
…equency (pandas-dev#53938)" This reverts commit dfc4c39.
…equency (pandas-dev#53938)" This reverts commit dfc4c39.
…equency (#53938)" (#55077) * Revert "BUG: Timestamp origin takes no effect in resample for 'MS' frequency (#53938)" This reverts commit dfc4c39. * note that origin only takes effect for tick frequencies * fixup doctest * move whatsnew to v2.1.2 --------- Co-authored-by: Thomas Li <[email protected]>
…effect in resample for 'MS' frequency (pandas-dev#53938)"
…es no effect in resample for 'MS' frequency (#53938)") (#55459) * Backport PR #55077: Revert "BUG: Timestamp origin takes no effect in resample for 'MS' frequency (#53938)" * fixup doctest * noop * fixup doctests --------- Co-authored-by: Marco Edward Gorelli <[email protected]> Co-authored-by: MarcoGorelli <[email protected]> Co-authored-by: Thomas Li <[email protected]>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.