Skip to content
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

BUG: fixes weekday for dates before 1752 #53795

Merged
merged 13 commits into from
Jun 27, 2023

Conversation

mcgeestocks
Copy link
Contributor

@mcgeestocks mcgeestocks commented Jun 22, 2023

@mcgeestocks mcgeestocks marked this pull request as ready for review June 22, 2023 16:57
@mcgeestocks
Copy link
Contributor Author

Approach was to switch to implementing Gauss's algorithm for weekday. Ended up being quite interesting.

@MarcoGorelli
Copy link
Member

nice work!

just got a couple of requests:

  • on the unit test side, could you add a couple of negative dates please?
  • could we also add a parametric test using hypothesis please, in which we compare the output against the standard library output? feel free to reach out if you're not sure how to do this

@mcgeestocks
Copy link
Contributor Author

Thanks @MarcoGorelli!

Haven't used Hypothesis before by tried to follow the patterns elsewhere in the codebase.
Also by "standard library" I assumed you were referring to datetime. Let me know if I missed the mark any what you were looking for.

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Jun 23, 2023

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 datetime, and compare Timestamp(datetime).weekday with datetime.strftime('%a')

something like that

@mcgeestocks
Copy link
Contributor Author

Sorry @MarcoGorelli did you see this push ca826d8 ?

I had already added the Hypothesis driven test before my pervious note, sorry for the confusion.

Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

doc/source/whatsnew/v2.1.0.rst Outdated Show resolved Hide resolved
pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
pandas/tests/scalar/timestamp/test_timestamp.py Outdated Show resolved Hide resolved
@MarcoGorelli MarcoGorelli added this to the 2.1 milestone Jun 26, 2023
@MarcoGorelli MarcoGorelli modified the milestones: 2.1, 2.0.3 Jun 26, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

@MarcoGorelli
Copy link
Member

CI failure is unrelated, but probably safer to wait til that's addressed before merging

@MarcoGorelli MarcoGorelli removed this from the 2.0.3 milestone Jun 27, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

@MarcoGorelli MarcoGorelli added this to the 2.0.3 milestone Jun 27, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a 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

@MarcoGorelli MarcoGorelli merged commit 20530b1 into pandas-dev:main Jun 27, 2023
34 checks passed
@lumberbot-app
Copy link

lumberbot-app bot commented Jun 27, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 20530b1f45045bfb95b0a79cda70fa7b804c9a6a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #53795: BUG: fixes weekday for dates before 1752'
  1. Push to a named branch:
git push YOURFORK 2.0.x:auto-backport-of-pr-53795-on-2.0.x
  1. Create a PR against branch 2.0.x, I would have named this PR:

"Backport PR #53795 on branch 2.0.x (BUG: fixes weekday for dates before 1752)"

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 Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

MarcoGorelli pushed a commit to MarcoGorelli/pandas that referenced this pull request Jun 27, 2023
MarcoGorelli added a commit that referenced this pull request Jun 27, 2023
…re 1752) (#53884)

Backport PR #53795: BUG: fixes weekday for dates before 1752

Co-authored-by: Conrad Mcgee Stocks <mcgee.stocks@gmail.com>
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request Jul 8, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: weekday incorrect for dates before 0000-03-01
3 participants