-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: fixes weekday for dates before 1752 #53795
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
Approach was to switch to implementing Gauss's algorithm for weekday. Ended up being quite interesting. |
nice work! just got a couple of requests:
|
Thanks @MarcoGorelli! Haven't used Hypothesis before by tried to follow the patterns elsewhere in the codebase. |
yup, that's right! one way to do it could be to make a dict with {0: 'Mon', 1: 'Tue', ...} and then generate a random datetime something like that |
Sorry @MarcoGorelli did you see this push ca826d8 ? I had already added the Hypothesis driven test before my pervious note, sorry for the confusion. |
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 one! just got a comment on the parametric test
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.
generally looks good, just got quite a few minor comments
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.
Looks good to me, thanks @mcgeestocks !
I've just moved this to the 2.0.3 milestone, as technically it's a regression introduced by version 2.0.0 (before, .dt.weekday
was always correct, as timestamps were only supported between about 1700 and 2300)
EDIT: requesting changes for now, see below
CI failure is unrelated, but probably safer to wait til that's addressed before merging |
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.
Although this looks correct, the implementation looks a bit different to what's linked to in https://en.wikipedia.org/wiki/Determination_of_the_day_of_the_week#Gauss's_algorithm
could you explain what the various letters you've introduced (e.g. e
) refer to please?
EDIT: seems like you've used the implementation from http://berndt-schwerdtfeger.de/wp-content/uploads/pdf/cal.pdf, which looks good. All good then, I'll add a note linking to that
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.
Right, I've noted the source of the implementation, have simplified slightly, and have expanded the parametric test to include negative dates (by comparing against numpy)
Very confident about this now - will merge today unless there's any objections, would be good to get this fix into 2.0.3
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
* switch to gauss algo * precommit cleanup * missed formatting and whatsnew * add hypothesis and negative test * update parametric test * requested changes * move whatsnew to 2.0.3 * post-merge fixup * note source of implementation, simplify, include negative dates in parametric test --------- Co-authored-by: MarcoGorelli <33491632+MarcoGorelli@users.noreply.github.com> Co-authored-by: Marco Edward Gorelli <marcogorelli@protonmail.com>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.