Skip to content

CLN: Use C-API for datetime.date #33222

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 3 commits into from
Apr 2, 2020

Conversation

ShaharNaveh
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jbrockmendel
Copy link
Member

jbrockmendel commented Apr 1, 2020

LGTM pending green

@TomAugspurger
Copy link
Contributor

For my own understanding, why do we prefer to use the C API? Is this likely to improve performance?

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Apr 1, 2020

Is this likely to improve performance?

@TomAugspurger Correct!


This is because when we use import and not cimport we are going through the python space, which is slower than pure C.

I have made POC so you could see to diffs:

date_vs_Cdate

Both are doing the exact same thing, only that the C-API datetime.date is much faster.

@@ -4,7 +4,6 @@ import time
import locale
import calendar
import re
import datetime
Copy link
Member

Choose a reason for hiding this comment

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

I think can just cimport cpython.datetime as datetime to minimize changes no? Didn't we move away from the import pattern here already in #32459

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Somehow slipped away from me

@WillAyd WillAyd added the Performance Memory or execution speed performance label Apr 1, 2020
@WillAyd WillAyd added this to the 1.1 milestone Apr 1, 2020
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm

@mroeschke mroeschke merged commit b5092d8 into pandas-dev:master Apr 2, 2020
@mroeschke
Copy link
Member

Thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the CLN-CAPI-datetime branch April 3, 2020 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants