Skip to content

update to holiday to help with GH#7070 #7913

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

Merged
merged 1 commit into from
Aug 11, 2014
Merged

update to holiday to help with GH#7070 #7913

merged 1 commit into from
Aug 11, 2014

Conversation

MichaelWS
Copy link
Contributor

I am working on a financial calendar and wanted to add the following functions to pandas.tseries.holiday in order to properly handle half days.

@jreback jreback added this to the 0.15.0 milestone Aug 4, 2014
@jreback
Copy link
Contributor

jreback commented Aug 4, 2014

looks reasonable (test failures though)

@rockg
Copy link
Contributor

rockg commented Aug 5, 2014

The right syntax would be h = Holiday("Easter Monday", month=1, day=1, offset=[Easter(), Day(1)]). However, I'm seeing a problem in python3 with the map objects. I think if we just wrap list() around the map in Holiday as it iterates through the offsets everything will work.

@MichaelWS
Copy link
Contributor Author

Thank you very much. Should I keep a test for defined holidays?

@rockg
Copy link
Contributor

rockg commented Aug 5, 2014

I don't see any harm in that.

@MichaelWS
Copy link
Contributor Author

Thanks for the help on this.

@MichaelWS
Copy link
Contributor Author

@rockg, I am still crashing on a python3 build. How did you intend to handle the two observations?

@MichaelWS
Copy link
Contributor Author

I will setup a python3 environment to test both going forward.

@rockg
Copy link
Contributor

rockg commented Aug 6, 2014

Yeah, I had to do the same. So many of my changes in the past needed
python3 specific fixes.

@jreback
Copy link
Contributor

jreback commented Aug 7, 2014

@MichaelWS looks fine, can you add a release note (just to document)

@MichaelWS
Copy link
Contributor Author

easy enough, should I put it in v0.15.0.txt?

@jreback
Copy link
Contributor

jreback commented Aug 7, 2014

yep

@@ -264,6 +264,8 @@ Enhancements


- ``PeriodIndex`` supports ``resolution`` as the same as ``DatetimeIndex`` (:issue:`7708`)
-``pandas.tseries.holiday`` has added support for additional holidays and ways to observe holidays.
-``pandas.tseries.holiday.Holiday`` now supports a list of offsets in Python3.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this doc-stringed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which one? The list of offsets was a bug that appeared in python3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was just an untested case/bug? put the pr reference in the release note

I meant does the doc string say it should accept a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an untested case. It takes a list or if its not a list it creates a list around what is inputted. However, two items never worked in python3.

I will edit the pr later today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok ping when done and green

@MichaelWS
Copy link
Contributor Author

all green, I added to the init so its clear

@jreback , thanks for the help.

@@ -149,7 +213,7 @@ def _apply_rule(self, dates):
offsets = self.offset

for offset in offsets:
dates = map(lambda d: d + offset, dates)
dates = list(map(lambda d: d + offset, dates))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is their a test that validates this now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GoodFriday/Easter tests he added validate that it works.

@jreback
Copy link
Contributor

jreback commented Aug 11, 2014

oh i see, ok then.

jreback added a commit that referenced this pull request Aug 11, 2014
update to holiday to help with GH#7070
@jreback jreback merged commit 7d3a0fa into pandas-dev:master Aug 11, 2014
@jreback
Copy link
Contributor

jreback commented Aug 11, 2014

thanks for the PR!

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

Successfully merging this pull request may close these issues.

3 participants