Skip to content

CLN: Remove days, seconds and microseconds properties from timedelta.pyx #18242

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

Closed
WillAyd opened this issue Nov 12, 2017 · 2 comments · Fixed by #18374
Closed

CLN: Remove days, seconds and microseconds properties from timedelta.pyx #18242

WillAyd opened this issue Nov 12, 2017 · 2 comments · Fixed by #18374
Labels
Clean Performance Memory or execution speed performance Timedelta Timedelta data type
Milestone

Comments

@WillAyd
Copy link
Member

WillAyd commented Nov 12, 2017

xref #18225

The days, seconds and microseconds properties defined in timedelta.pyx could potentially be removed as they are defined in the Cython superclass. See Jeff's comments in the linked PR for caveats that need to be explored before totally committing to this.

@jreback jreback added Clean Difficulty Intermediate Performance Memory or execution speed performance Timedelta Timedelta data type and removed Effort Medium labels Nov 12, 2017
@jreback jreback added this to the Next Major Release milestone Nov 12, 2017
@jreback jreback changed the title CLN: Remove days, seconds and microseconds properties from timestamp.pyx CLN: Remove days, seconds and microseconds properties from timedelta.pyx Nov 12, 2017
@WillAyd
Copy link
Member Author

WillAyd commented Nov 19, 2017

Here's what I got from ASV after removing those properties. Note that there is no nanoseconds property in CPython's source, so that will need to remain here for the time being

   before         after       ratio
   215±4ns        167±4ns     0.78  timedelta.TimedeltaProperties.time_timedelta_days
   212±4ns        155±2ns     0.73  timedelta.TimedeltaProperties.time_timedelta_microseconds
   210±3ns        149±3ns     0.71  timedelta.TimedeltaProperties.time_timedelta_seconds

From a few runs it looks like me like savings could be 20-25%. You've called out before that this will make our docstrings inconsistent and it is a little clunky since nanoseconds would still be around. On the flip side, it is less code for pandas and gives a slight performance boost. Worth a PR?

@jreback
Copy link
Contributor

jreback commented Nov 19, 2017

nanoseconds is unsupported in CPython, so that has to remain.

I am not sure the doc-strings are a big deal, Timedelta doc-strings are a bit sparse ATM as well :>

so if you want to do this change would be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Performance Memory or execution speed performance Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants