BUG: Increase range of dates for holiday calculations #27790
Conversation
|
I have no problem with wider. I was just concerned there might be some performance hit with a wider date range. My usage is primarily calculating payment dates for bonds and swaps, where going out to 50 or 60 years will be fine in general, but in principle it would certainly be better to go back to, say, 1900 and out to, say, 2200. We have certainly seen bonds issued with 100 years to maturity.
… On Aug 6, 2019, at 1:57 PM, gfyoung ***@***.***> wrote:
@gfyoung commented on this pull request.
In pandas/tseries/holiday.py <#27790 (comment)>:
> @@ -345,8 +345,8 @@ class AbstractHolidayCalendar(metaclass=HolidayCalendarMetaClass):
"""
rules = [] # type: List[Holiday]
- start_date = Timestamp(datetime(1970, 1, 1))
- end_date = Timestamp(datetime(2030, 12, 31))
+ start_date = Timestamp(datetime(1960, 1, 1))
+ end_date = Timestamp(datetime(2080, 12, 31))
Why not wider? 🤷♂
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDUUTXZPOBYVOIYDCWDQDHQVBA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAYHE2Y#pullrequestreview-271610475>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDSG2UXRGCMV7Z2ZHSTQDHQVBANCNFSM4IJ2BU2Q>.
|
|
Fine with me. Do you need me to make the edit, or will you take care of it at your end?
… On Aug 6, 2019, at 2:04 PM, gfyoung ***@***.***> wrote:
@gfyoung commented on this pull request.
In pandas/tseries/holiday.py <#27790 (comment)>:
> @@ -345,8 +345,8 @@ class AbstractHolidayCalendar(metaclass=HolidayCalendarMetaClass):
"""
rules = [] # type: List[Holiday]
- start_date = Timestamp(datetime(1970, 1, 1))
- end_date = Timestamp(datetime(2030, 12, 31))
+ start_date = Timestamp(datetime(1960, 1, 1))
+ end_date = Timestamp(datetime(2080, 12, 31))
I have no problem with wider. I was just concerned there might be some performance hit with a wider date range. My usage is primarily calculating payment dates for bonds and swaps, where going out to 50 or 60 years will be fine in general, but in principle it would certainly be better to go back to, say, 1900 and out to, say, 2200. We have certainly seen bonds issued with 100 years to maturity.
Let's go with 1900 - 2200
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDWNHFCTT5A2352LNFDQDHRPDA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAYIBFA#discussion_r311274010>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDWEGI5PG2RE7PX6KL3QDHRPDANCNFSM4IJ2BU2Q>.
|
|
Was there any performance impact? |
|
Are there holidays that were instituted between 1900 and 1970 that will incorrectly be retroactively celebrated? |
|
That’s a good question. I just checked Memorial Day, and according to Wikipedia, while it’s now the last Monday in May, it was celebrated on May 30 from 1868 to 1970. So it does look like some more work might be needed to get old holidays right.
… On Aug 6, 2019, at 3:32 PM, jbrockmendel ***@***.***> wrote:
Are there holidays that were instituted between 1900 and 1970 that will incorrectly be retroactively celebrated?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDTWBST6543GEIKPKPDQDH3YJA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3WUZDQ#issuecomment-518868110>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDTFCVRNTQ27ZDHIIO3QDH3YJANCNFSM4IJ2BU2Q>.
|
|
We’re OK on Labor Day - it’s been the first Monday in September since 1894.
… On Aug 6, 2019, at 3:39 PM, Richard Stanton ***@***.***> wrote:
That’s a good question. I just checked Memorial Day, and according to Wikipedia, while it’s now the last Monday in May, it was celebrated on May 30 from 1868 to 1970. So it does look like some more work might be needed to get old holidays right.
> On Aug 6, 2019, at 3:32 PM, jbrockmendel ***@***.*** ***@***.***>> wrote:
>
> Are there holidays that were instituted between 1900 and 1970 that will incorrectly be retroactively celebrated?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDTWBST6543GEIKPKPDQDH3YJA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3WUZDQ#issuecomment-518868110>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDTFCVRNTQ27ZDHIIO3QDH3YJANCNFSM4IJ2BU2Q>.
>
|
|
I’m fine with just extending the end date for now.
…Sent from my mobile device
On Aug 6, 2019, at 3:41 PM, Richard Stanton ***@***.***> wrote:
We’re OK on Labor Day - it’s been the first Monday in September since 1894.
> On Aug 6, 2019, at 3:39 PM, Richard Stanton ***@***.***> wrote:
>
> That’s a good question. I just checked Memorial Day, and according to Wikipedia, while it’s now the last Monday in May, it was celebrated on May 30 from 1868 to 1970. So it does look like some more work might be needed to get old holidays right.
>
>
>> On Aug 6, 2019, at 3:32 PM, jbrockmendel ***@***.***> wrote:
>>
>> Are there holidays that were instituted between 1900 and 1970 that will incorrectly be retroactively celebrated?
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub, or mute the thread.
>>
>
|
|
When I try a simple “git push”, I get the following error:
fatal: The current branch holiday_update has no upstream branch.
To push the current branch and set the remote as upstream, use
git push --set-upstream origin holiday_update
What next…?
… On Aug 7, 2019, at 1:17 PM, gfyoung ***@***.***> wrote:
@gfyoung commented on this pull request.
In pandas/tseries/holiday.py <#27790 (comment)>:
> @@ -345,8 +345,8 @@ class AbstractHolidayCalendar(metaclass=HolidayCalendarMetaClass):
"""
rules = [] # type: List[Holiday]
- start_date = Timestamp(datetime(1970, 1, 1))
- end_date = Timestamp(datetime(2030, 12, 31))
+ start_date = Timestamp(datetime(1960, 1, 1))
+ end_date = Timestamp(datetime(2080, 12, 31))
Even better: just push new changes to your branch. It will update this PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDQIB3JHLO7H7IGGHRDQDMUT3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA4YGJA#discussion_r311743707>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDXIIB4IIFUVAK4RSLDQDMUT3ANCNFSM4IJ2BU2Q>.
|
|
Does `git push --set-upstream origin holiday_update` work?
…On Wed, Aug 7, 2019 at 3:50 PM rhstanton ***@***.***> wrote:
When I try a simple “git push”, I get the following error:
fatal: The current branch holiday_update has no upstream branch.
To push the current branch and set the remote as upstream, use
git push --set-upstream origin holiday_update
What next…?
> On Aug 7, 2019, at 1:17 PM, gfyoung ***@***.***> wrote:
>
> @gfyoung commented on this pull request.
>
> In pandas/tseries/holiday.py <
#27790 (comment)>:
>
> > @@ -345,8 +345,8 @@ class
AbstractHolidayCalendar(metaclass=HolidayCalendarMetaClass):
> """
>
> rules = [] # type: List[Holiday]
> - start_date = Timestamp(datetime(1970, 1, 1))
> - end_date = Timestamp(datetime(2030, 12, 31))
> + start_date = Timestamp(datetime(1960, 1, 1))
> + end_date = Timestamp(datetime(2080, 12, 31))
> Even better: just push new changes to your branch. It will update this
PR.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub <
#27790?email_source=notifications&email_token=AAQNZDQIB3JHLO7H7IGGHRDQDMUT3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA4YGJA#discussion_r311743707>,
or mute the thread <
https://github.com/notifications/unsubscribe-auth/AAQNZDXIIB4IIFUVAK4RSLDQDMUT3ANCNFSM4IJ2BU2Q
>.
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#27790?email_source=notifications&email_token=AAKAOIQI3G5JNWNOCNT7AWDQDMYS3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZVNJY#issuecomment-519263911>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIVDTWEZLRK4JQ6QGKLQDMYS3ANCNFSM4IJ2BU2Q>
.
|
|
I can’t tell from the description whether that does something good or bad (like changing the identity of the upstream repository). I was hoping someone could tell me that this was a good thing to try…
… On Aug 7, 2019, at 1:58 PM, Tom Augspurger ***@***.***> wrote:
Does `git push --set-upstream origin holiday_update` work?
On Wed, Aug 7, 2019 at 3:50 PM rhstanton ***@***.***> wrote:
> When I try a simple “git push”, I get the following error:
>
> fatal: The current branch holiday_update has no upstream branch.
> To push the current branch and set the remote as upstream, use
>
> git push --set-upstream origin holiday_update
>
> What next…?
>
> > On Aug 7, 2019, at 1:17 PM, gfyoung ***@***.***> wrote:
> >
> > @gfyoung commented on this pull request.
> >
> > In pandas/tseries/holiday.py <
> #27790 (comment)>:
> >
> > > @@ -345,8 +345,8 @@ class
> AbstractHolidayCalendar(metaclass=HolidayCalendarMetaClass):
> > """
> >
> > rules = [] # type: List[Holiday]
> > - start_date = Timestamp(datetime(1970, 1, 1))
> > - end_date = Timestamp(datetime(2030, 12, 31))
> > + start_date = Timestamp(datetime(1960, 1, 1))
> > + end_date = Timestamp(datetime(2080, 12, 31))
> > Even better: just push new changes to your branch. It will update this
> PR.
> >
> > —
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub <
> #27790?email_source=notifications&email_token=AAQNZDQIB3JHLO7H7IGGHRDQDMUT3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA4YGJA#discussion_r311743707>,
> or mute the thread <
> https://github.com/notifications/unsubscribe-auth/AAQNZDXIIB4IIFUVAK4RSLDQDMUT3ANCNFSM4IJ2BU2Q
> >.
> >
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#27790?email_source=notifications&email_token=AAKAOIQI3G5JNWNOCNT7AWDQDMYS3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZVNJY#issuecomment-519263911>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIVDTWEZLRK4JQ6QGKLQDMYS3ANCNFSM4IJ2BU2Q>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDXBROVIQVWWDX2GVNDQDMZOTA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZV62A#issuecomment-519266152>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDRNRGHSUDL6YH7UUJDQDMZOTANCNFSM4IJ2BU2Q>.
|
|
is what I do every time I push a new branch. The thing I'm confused about is how you pushed the first time? |
|
I think I did
git push origin holiday_update
… On Aug 7, 2019, at 2:41 PM, jbrockmendel ***@***.***> wrote:
git push --set-upstream origin holiday_update
is what I do every time I push a new branch. The thing I'm confused about is how you pushed the first time?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#27790?email_source=notifications&email_token=AAQNZDTALEVSCDM4IZY7NW3QDM6P3A5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZZFLQ#issuecomment-519279278>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQNZDTTV2IDSFC5NK7MV5TQDM6P3ANCNFSM4IJ2BU2Q>.
|
|
can you merge master, also I think we would want a test for this. |
Is that something I can do, or does someone with greater permissions need to do this? |
|
You can merge master:
https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request
…On Thu, Sep 19, 2019 at 12:41 PM rhstanton ***@***.***> wrote:
can you merge master, also I think we would want a test for this.
Is that something I can do, or does someone with greater permissions need
to do this?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#27790?email_source=notifications&email_token=AAKAOITYRGUOBXNGWHQR3U3QKO2UJA5CNFSM4IJ2BU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7EINEY#issuecomment-533235347>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIR76TUDZJLQ6W6LUNDQKO2UJANCNFSM4IJ2BU2Q>
.
|
|
Hello @rhstanton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-10-18 17:17:22 UTC |
|
I've added a test and pushed back to github, but I'm not sure how to merge my new code into master... |
|
Github tells me "This branch has no conflicts with the base branch. Only those with write access to this repository can merge pull requests." How do I get write access? |
Write access is restricted to maintainers. In order for us to merge your PR, we do need all tests to pass first! |
It’s true that two tests failed, but they had nothing to do with my edits! |
That is true, but as a rule, we do want to merge with all tests passing. In this case, to "fix" the issue, you should rebase or merge |
| workDay = offsets.CustomBusinessDay(calendar=cal) | ||
| Sat_before_Labor_Day_2031 = to_datetime('2031-08-30') | ||
| next_working_day = Sat_before_Labor_Day_2031 + 0 * workDay | ||
| assert(next_working_day == to_datetime('2031-09-02')) |
There was a problem hiding this comment.
And while you're at it: can you remove the parentheses here?
We use bare assert statements (see the test above).
There was a problem hiding this comment.
Will do. I'll wait until tomorrow, though. Merging master had no effect just now, so it seems the errors are in the current version.
There was a problem hiding this comment.
After pushing , the checks now give me an error message like this:
##[error]Bash exited with code '1'.
But I have no idea what to do about fixing this.
There was a problem hiding this comment.
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=17967
It looks like the black pandas lint check is failing (https://pypi.org/project/black/).
You are so close!
There was a problem hiding this comment.
Reformatted that file using black. Let's see if it works now. I'm hoping it's a bit quicker next time...
There was a problem hiding this comment.
It passed all tests!
|
Can you add a whatsnew? Does this have any performance impacts as questioned by @jbrockmendel ? |
|
Added an entry to whatsnew. No obvious performance impact in my uses. What still needs doing? |
doc/source/whatsnew/v0.25.2.rst
Outdated
| @@ -40,6 +40,9 @@ Other | |||
| - Compatibility with Python 3.8 in :meth:`DataFrame.query` (:issue:`27261`) | |||
| - Fix to ensure that tab-completion in an IPython console does not raise | |||
| warnings for deprecated attributes (:issue:`27900`). | |||
| - Fix :class:`AbstractHolidayCalendar` to return correct results for | |||
There was a problem hiding this comment.
@rhstanton as a last check can you run the ASV benchmarks in asv/benchmarks/offset.py and post results here? If no regressions I think good to merge
|
lgtm. |
|
No output: (pandas_dev) Richards-Mac-Pro:benchmarks stanton$ python offset.py |
you need to actually run the asv benchmarks: https://dev.pandas.io/docs/development/contributing.html#running-the-performance-test-suite |
|
Now I know... Not sure how much of this matters, so here's the entire output: (pandas_dev) Richards-Mac-Pro:asv_bench stanton$ asv continuous -f 1.1 upstream/master holiday_update -b ^offset [ 55.00%] ··· offset.OffestDatetimeArithmetic.time_add ok [ 57.50%] ··· offset.OffestDatetimeArithmetic.time_add_10 ok [ 60.00%] ··· offset.OffestDatetimeArithmetic.time_apply ok [ 62.50%] ··· offset.OffestDatetimeArithmetic.time_apply_np_dt64 ok [ 65.00%] ··· offset.OffestDatetimeArithmetic.time_subtract ok [ 67.50%] ··· offset.OffestDatetimeArithmetic.time_subtract_10 ok [ 70.00%] ··· offset.OffsetDatetimeIndexArithmetic.time_add_offset ok [ 72.50%] ··· offset.OffsetSeriesArithmetic.time_add_offset ok [ 75.00%] ··· offset.OnOffset.time_on_offset ok [ 75.00%] · For pandas commit 709436d <holiday_update^2> (round 2/2): [ 80.00%] ··· offset.OffestDatetimeArithmetic.time_add ok [ 82.50%] ··· offset.OffestDatetimeArithmetic.time_add_10 ok [ 85.00%] ··· offset.OffestDatetimeArithmetic.time_apply ok [ 87.50%] ··· offset.OffestDatetimeArithmetic.time_apply_np_dt64 ok [ 90.00%] ··· offset.OffestDatetimeArithmetic.time_subtract ok [ 92.50%] ··· offset.OffestDatetimeArithmetic.time_subtract_10 ok [ 95.00%] ··· offset.OffsetDatetimeIndexArithmetic.time_add_offset ok [ 97.50%] ··· offset.OffsetSeriesArithmetic.time_add_offset ok [100.00%] ··· offset.OnOffset.time_on_offset ok BENCHMARKS NOT SIGNIFICANTLY CHANGED. |
|
Thanks @rhstanton very nice PR |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff