Skip to content

Conversation

@clearf
Copy link
Contributor

@clearf clearf commented Jun 4, 2015

closes #9760

Fixing MLK day not observed before 1986, per #10278 (comment)

@clearf
Copy link
Contributor Author

clearf commented Jun 5, 2015

Hi,
I also noticed what appear to be two additional inconsistencies.

  • Memorial day is the last Monday in May. Code as before would not work if May 31st were a Monday.
  • New Years Day is not offset to the nearest workday in the US calendar as far as I can tell.

@rockg
Copy link
Contributor

rockg commented Jun 5, 2015

Memorial day is already fixed in #9763 but it needs a test before it can be pushed. Maybe it's best to incorporate the test there into this PR. Also, all your other changes need tests.

@rockg
Copy link
Contributor

rockg commented Jun 5, 2015

Also, New Year's Day is offset to the nearest workday for US Holidays. https://www.opm.gov/policy-data-oversight/snow-dismissal-procedures/federal-holidays/. Every other calendar used for holiday work uses a similar convention and so I think it makes sense to leave the rule as it is. These rules are mostly meant to be examples and cover the standard use cases so it makes sense to leave New Years going to nearest_workday.

@clearf
Copy link
Contributor Author

clearf commented Jun 5, 2015

OK, thanks. That makes a lot of sense. And you're right re: federal
holidays. I misread the spec. And they are a good example.

If Memorial Day is fixed elsewhere, then it seems like only MLK is the only
change that should actually be fixed. Sorry for the churn.

Thanks,
Chris

On Fri, Jun 5, 2015 at 8:58 AM, rockg [email protected] wrote:

Also, New Year's Day is offset to the nearest workday for US Holidays.
https://www.opm.gov/policy-data-oversight/snow-dismissal-procedures/federal-holidays/.
Every other calendar used for holiday work uses a similar convention and so
I think it makes sense to leave the rule as it is. These rules are mostly
meant to be examples and cover the standard use cases so it makes sense to
leave New Years going to nearest_workday.


Reply to this email directly or view it on GitHub
#10282 (comment).

Chris Clearfield

@jreback
Copy link
Contributor

jreback commented Jun 26, 2015

@clearf can you add some tests?

@clearf
Copy link
Contributor Author

clearf commented Jul 4, 2015

@jreback sorry for the delay (camping). Happy to add some tests.

Some pointers to the test/build process would be great.

Thanks,
Chris

On Fri, Jun 26, 2015 at 4:33 PM, jreback [email protected] wrote:

@clearf https://github.com/clearf can you add some tests?


Reply to this email directly or view it on GitHub
#10282 (comment).

Chris Clearfield

@jreback
Copy link
Contributor

jreback commented Jul 4, 2015

@clearf
Copy link
Contributor Author

clearf commented Jul 4, 2015

Great, thanks.

On Fri, Jul 3, 2015 at 6:09 PM, jreback [email protected] wrote:

http://pandas.pydata.org/pandas-docs/stable/contributing.html


Reply to this email directly or view it on GitHub
#10282 (comment).

Chris Clearfield

@jreback jreback added the Frequency DateOffsets label Jul 6, 2015
@clearf
Copy link
Contributor Author

clearf commented Aug 5, 2015

@jreback Per above, added tests for MLK day. Also added tests for Memorial day fix, though my implementation is slightly different than that referenced in #9763

Sorry it took so long.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can put the imports at the top of the file with the rest of them.

@jreback
Copy link
Contributor

jreback commented Aug 6, 2015

looks good. couple of minor comments, then pls squash. ping when green.

@jreback jreback added this to the 0.17.0 milestone Aug 6, 2015
@clearf
Copy link
Contributor Author

clearf commented Aug 6, 2015

@jreback, OK, comments added and imports moved. All checks passed.

And my apologies: I squashed my most recent commits, but could not figure out how to squash my commits from early June that predated the merge. Let me know if you know how to do this and I can fix.

@jreback
Copy link
Contributor

jreback commented Aug 10, 2015

@clearf
Copy link
Contributor Author

clearf commented Aug 10, 2015

Ok, thanks.

On Mon, Aug 10, 2015 at 04:04 Jeff Reback [email protected] wrote:

contributing docs here:
http://pandas.pydata.org/pandas-docs/stable/contributing.html

pls rebase/squash


Reply to this email directly or view it on GitHub
#10282 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

add on #9760 as well here

@clearf
Copy link
Contributor Author

clearf commented Aug 13, 2015

[edit] @jreback Jeff, I think I rebased all my commits correctly and pushed. But I still see 226 files changed, when obviously I only changed a few. I'm still not sure if I'm doing this correctly. I think some of these are from my upstream fetch?

Sorry, this is my first time doing this. I'm happy to start from scratch with a new PR.

@clearf clearf force-pushed the master branch 2 times, most recently from 70b0d70 to 38b5418 Compare August 13, 2015 02:25
@jreback
Copy link
Contributor

jreback commented Aug 15, 2015

you need to git rebase -i origin/master

@clearf
Copy link
Contributor Author

clearf commented Aug 16, 2015

@jreback And... I think we're there. Thanks for the guidance/patience.

@jreback
Copy link
Contributor

jreback commented Aug 18, 2015

merged via 8ccdf48

thanks!

@clearf
Copy link
Contributor Author

clearf commented Aug 18, 2015

Small victories. :)
On Tue, Aug 18, 2015 at 04:09 Jeff Reback [email protected] wrote:

Closed #10282 #10282.


Reply to this email directly or view it on GitHub
#10282 (comment).

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

Labels

Frequency DateOffsets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

USMemorialDay defined incorrectly.

3 participants