-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF: add shortcut to Timedelta constructor #31070
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
Same as in the Timestamp shortcut, we could hoist the shortcut to the very top of |
What kind of performance gain does this show? |
@WillAyd About 20% on 10 000 000 iterations. |
# Test for #30543 | ||
expected = pd.Timedelta(np.timedelta64(1, "s")) | ||
result = pd.Timedelta(expected) | ||
assert result is expected |
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.
I'm not sure about this expectation - we don't do this for Timestamps so not sure should do it for Timedeltas as well but maybe @jschendel has thoughts
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.
There is a test for the constructor identity too, in the Timestamp shortcut PR #30676. Sorry for posting the Timedelta one before that one gets merged. It's just that the discussion was mostly done there, so I thought I'd get on the Timedelta one too.
If it's inconvenient, then we can put this one on hold until #30676 gets merged, and then I can just copy the approach we settle on there.
def setup(self): | ||
self.nptimedelta64 = np.timedelta64(3600) | ||
self.dttimedelta = datetime.timedelta(seconds=3600) | ||
self.td = Timedelta(3600, unit="s") |
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.
Since we are here, let's add a benchmark for making Timedelta out of Timedelta, and I think we should also tweak existing tests. Currently, we benchmark both our method and np.timedelta64
and datetime.timedelta
at the same time. We should move the initialization into setup
, in my opinion. It's a bit nitpicky, of course, since both np.timedelta64
and datetime.timedelta
are unlikely to change speed in the future, but still.
thanks @AlexKirko |
WIP until #30676 gets merged.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
This implements a shortcut in the Timedelta constructor to cut down on processing if Timedelta is passed. We still need to check if any other args are passed. Then, if a Timedelta with no other kwargs was passed, we just return that same Timedelta.
A test is added to check that the Timedelta is still the same object.