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

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

Closed
WillAyd opened this Issue Nov 12, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@WillAyd
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 this to the Next Major Release milestone Nov 12, 2017

@jreback jreback changed the title from CLN: Remove days, seconds and microseconds properties from timestamp.pyx to CLN: Remove days, seconds and microseconds properties from timedelta.pyx Nov 12, 2017

@WillAyd

This comment has been minimized.

Member

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

This comment has been minimized.

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.

WillAyd added a commit to WillAyd/pandas that referenced this issue Nov 19, 2017

@jreback jreback modified the milestones: Next Major Release, 0.22.0 Nov 20, 2017

jreback added a commit that referenced this issue Nov 20, 2017

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