Skip to content

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

Merged
merged 8 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions asv_bench/benchmarks/tslibs/timedelta.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@


class TimedeltaConstructor:
def setup(self):
self.nptimedelta64 = np.timedelta64(3600)
self.dttimedelta = datetime.timedelta(seconds=3600)
self.td = Timedelta(3600, unit="s")
Copy link
Member Author

@AlexKirko AlexKirko Jan 23, 2020

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.


def time_from_int(self):
Timedelta(123456789)

Expand All @@ -28,10 +33,10 @@ def time_from_components(self):
)

def time_from_datetime_timedelta(self):
Timedelta(datetime.timedelta(days=1, seconds=1))
Timedelta(self.dttimedelta)

def time_from_np_timedelta(self):
Timedelta(np.timedelta64(1, "ms"))
Timedelta(self.nptimedelta64)

def time_from_string(self):
Timedelta("1 days")
Expand All @@ -42,6 +47,9 @@ def time_from_iso_format(self):
def time_from_missing(self):
Timedelta("nat")

def time_from_pd_timedelta(self):
TimeDelta(self.td)


class TimedeltaProperties:
def setup_cache(self):
Expand Down
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.1.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ Deprecations

Performance improvements
~~~~~~~~~~~~~~~~~~~~~~~~

- Performance improvement in :class:`Timedelta` constructor (:issue:`30543`)
-
-

Expand Down
7 changes: 6 additions & 1 deletion pandas/_libs/tslibs/timedeltas.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,12 @@ class Timedelta(_Timedelta):
"represent unambiguous timedelta values durations."
)

if isinstance(value, Timedelta):
# GH 30543 if pd.Timedelta already passed, return it
# check that only value is passed
if (isinstance(value, Timedelta) and unit is None and
len(kwargs) == 0):
return value
elif isinstance(value, Timedelta):
value = value.value
elif isinstance(value, str):
if len(value) > 0 and value[0] == 'P':
Expand Down
7 changes: 7 additions & 0 deletions pandas/tests/indexes/datetimes/test_constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -950,3 +950,10 @@ def test_datetimeindex_constructor_misc(self):
)
assert len(idx1) == len(idx2)
assert idx1.freq == idx2.freq


def test_timedelta_constructor_identity():
# Test for #30543
expected = pd.Timedelta(np.timedelta64(1, "s"))
result = pd.Timedelta(expected)
assert result is expected
Copy link
Member

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

Copy link
Member Author

@AlexKirko AlexKirko Jan 16, 2020

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.