PERF: add shortcut to Timestamp constructor #30676

AlexKirko · 2020-01-04T07:19:38Z

closes PERF: Timestamp/Timedelta constructors when passed a Timestamp/Timedelta #30543
tests added 1 / passed 1
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This implements a shortcut in the Timestamp constructor to cut down on processing if Timestamp is passed. We still need to check if the timezone was passed correctly. Then, if a Timestamp was passed, and there is no timezone, we just return that same Timestamp.
A test is added to check that the Timestamp is still the same object.

PR for timedelta to be added once I confirm that this is the approach we want to go with.

AlexKirko · 2020-01-04T08:18:47Z

Simply adding a shortcut breaks date_range in an unintuitive way (see test TestDatetimeIndexTimezones.test_dti_construction_ambiguous_endpoint). I'll look deeper into this today to see why date_range no longer takes into account daylight savings with my fix.

jbrockmendel · 2020-01-04T16:05:04Z

pandas/_libs/tslibs/timestamps.pyx

@@ -389,7 +389,10 @@ class Timestamp(_Timestamp):
            # User passed tzinfo instead of tz; avoid silently ignoring
            tz, tzinfo = tzinfo, None

-        if isinstance(ts_input, str):
+        if isinstance(ts_input, Timestamp) and tz is None:


you're going to need to check that all the other kwargs are None

Thanks, I'll change this, but first I need to solve #24329, because right now we rely on Timestamp constructor changing ts.value when called on a localized DST Timestamp (this is why the tests for this PR break currently).

Depending on how digging through the code goes, this PR might also take care of #24329, but I still need a couple days to analyze tz_localize, pd.date_range, and the Timestamp constructor.
Update: a separate PR is needed. Dealing with #24329 in PR #30995, will return here after that gets done.

@jbrockmendel Solved #24329. Now this PR can be reviewed again.

pep8speaks · 2020-01-15T18:28:12Z

Hello @AlexKirko! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-24 09:24:16 UTC

AlexKirko · 2020-01-15T20:43:32Z

pandas/tests/indexes/datetimes/test_timezones.py

-                "2019-03-10 01:00",
-                marks=pytest.mark.xfail(reason="GH 24329"),
-            ),
+            ["dateutil/US/Pacific", "shift_backward", "2019-03-10 01:00"],


This shortcut, along with the fix to #24329 makes this exception no longer necessary, as a correct value gets returned without an error.
UPDATE: #31155 made the check succeed with dateutil.__version__ >= 2.7.0 . With this shortcut, the exception is not necessary with any version of dateutil. What happened before was that during making a date_range we would call pd.Timestamp twice and this would alter the object in case of a dateutil timezone near a winter/summer DST switch, which would make the test fail. #31155 made sure that the object didn't get altered with an updated version of dateutil and this shortcut eliminates the danger of the Timestamp being altered altogether, because the shortcut simply returns that same object.

AlexKirko · 2020-01-15T20:44:32Z

pandas/_libs/tslibs/timestamps.pyx

@@ -379,6 +379,8 @@ class Timestamp(_Timestamp):
        _date_attributes = [year, month, day, hour, minute, second,
                            microsecond, nanosecond]

+        _non_ts_attributes = [freq, tz, unit, tzinfo] + _date_attributes


I suggest we check for emtpy kwargs similar to the way we check for empty date attributes.

AlexKirko · 2020-01-15T20:44:55Z

pandas/_libs/tslibs/timestamps.pyx

+        # GH 30543 if pd.Timestamp already passed, return it
+        # check that only ts_input is passed
+        if (isinstance(ts_input, Timestamp) and not
+                any(arg is not None for arg in _non_ts_attributes)):


The same as with _date_attributes.

This makes sense. One question since we're focusing on performance: does it make a difference if you write out the verbose and foo is None and bar is None and baz is None...? i have a suspicion that cython doesnt optimize this list comprehension

@jbrockmendel Let's test it then.
The current implementation:

>>> ts = pd.Timestamp("2019-01-01") >>> timeit.timeit(lambda: pd.Timestamp(ts), number=10000000) 5.860465700000002

And now with this implementation:

if (isinstance(ts_input, Timestamp) and freq is None and tz is None and unit is None and year is None and month is None and day is None and hour is None and minute is None and second is None and microsecond is None and nanosecond is None and tzinfo is None): return ts_input

>>> ts = pd.Timestamp("2019-01-01") >>> timeit.timeit(lambda: pd.Timestamp(ts), number=10000000) 4.43885800000001

I've also tested this when we supply other arguments, but the overhead (or early exit from the shortcut if condition) aren't noticeable then, because the non-shortcut Timestamp is so much slower.
I think you are right, and we probably incur the overhead because Cython doesn't explode the list comprehension and instead calls the Python API.
To be honest, the exact reasoning doesn't matter, because I don't think we'll find anything faster than chaining arg is None and checks.

AlexKirko · 2020-01-15T20:46:37Z

I suppose I should also move this to whatsnew for version 1.1.0?
Update: moved it since we already have the 1.0.0 release candidate.

AlexKirko · 2020-01-16T07:24:54Z

pandas/_libs/tslibs/timestamps.pyx

+                month is None and day is None and hour is None and
+                minute is None and second is None and
+                microsecond is None and nanosecond is None and
+                tzinfo is None):


@jbrockmendel This appears the way to go if we need maximum performance.
We do lose a bit of speed (between 10 and 20 percent) because we implement the shortcut after errorchecks and _date_attributes.
Is this fine or should we hoist it after (or before) _date_attributes = [year, month, day, hour, minute, second, microsecond, nanosecond]? I think the current way is tidier, but it's a tradeoff.

I'm not sure all of these extra checks are worth adding to improve perf when passing a Timestamp to a Timestamp constructor - @AlexKirko how often are you expecting that to actually happen?

how often are you expecting that to actually happen?

For internal usage there are a lot of places where we do:

if isinstance(obj, (datetime, np.datetime64)): obj = Timestamp(obj)

That's not exactly the usage being checked here, but could benefit in the same way from an optimized no-other-arguments-passed check.

Admittedly, I don't know the library well enough to comment on internal usage, but @jbrockmendel has already done that.

However, what I've done repeatedly in my own projects when on a deadline is take a Dataframe column or a list and just cast the elements into the type I need, trusting that if it already is that class, the performance loss won't be noticeable in the larger program (I do lots of ad-hoc data science modeling). I don't think it's as much of a problem in production-quality code, but a lot of people I work with use pandas to quickly preprocess data for sklearn.

You tend to rely on being able to cut corners when working with a well-supported package, and, currently, calling Timestamp on a Timestamp is more than 10 000 times slower than the proposed shortcut, which can be a nasty shock for someone expecting to just blitz through type conversions during data wrangling.

@WillAyd We could make the code a bit more compact with what was proposed in 4c9eb70, which you can look up above, but this incurs about a 25% performance loss. I believe that if we do the shortcut, might as well add extra two lines. It's a bit grating but the gain is worth it, I think.

can you add a comment to the effect of "we do this verbose thing because cython wont optimize a list comprehension (as of cython 0.29.x)"

@jbrockmendel Added the comment you requested.

jreback · 2020-01-17T11:18:14Z

when you say cast? are you doing an astype or some sort of loop? can i show the original example perf issue

AlexKirko · 2020-01-18T07:25:38Z

When I say "cast" mean a timeit loop of pd.Timestamp(ts) in the python console. Something along the lines of:

ts = pd.Timestamp(100)
timeit(lambda: pd.Timestamp(ts), number=10000000)

@jreback Thanks for suggesting designing and running more tests. I went ahead and did that.

TLDR: depending on the usage conditions, the shortcut can speed up conversion to Timestamp to very different degrees. We speed things up about:

>10 000 times in a timeit call in the console (testing done previously).
~7 times in a %timeit call in Jupyter Notebook
~30% when using pd.Series.apply
Nothing when using pd.Series.astype.

In addition:

Hoisting the check to the very top of __new__speeds up loop performance by about 20% and doesn't speed up anything else.
Replacing all the is not None checks with not any on a list comprehension slows down loop performance by about 100%, slows down apply a bit, and doesn't impact astype.

I think that implementing the shortcut is definitely worth it. As much as we might prefer it to not be true, some people will always loop when there is a better solution available (and sometimtes it's not available). I don't think moving the check to the very top of the function is worth it, as that is very ugly, but using expanded is not None gives a large performance boost in loops, so I think we should keep it.

What do you think?

The performance tests code and the results are below.

If we run the original perf issue code on master:

IN:
ts = pd.Timestamp('2020')
%timeit pd.Timestamp(ts)
OUT:
2.17 µs ± 168 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

And on the current commit of the PR branch (bf08cc8):

IN:
ts = pd.Timestamp('2020')
%timeit pd.Timestamp(ts)
OUT:
372 ns ± 31 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

So with original issue code, the shortcut runs about 7 times faster.
This is, of course, the worst possible case, but loops might be used only by somebody just starting with pandas and Python in general.

Let's test this further.
On master:

IN:
t_ser = pd.Series(range(100000))
t_ser = t_ser.astype('<M8[ns]')
%timeit t_ser.apply(lambda x: pd.Timestamp(x))
%timeit t_ser.astype('<M8[ns]')
OUT:
634 ms ± 25.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
146 µs ± 2.73 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

And on the current commit of the PR branch (bf08cc8):

IN:
t_ser = pd.Series(range(100000))
t_ser = t_ser.astype('<M8[ns]')
%timeit t_ser.apply(lambda x: pd.Timestamp(x))
%timeit t_ser.astype('<M8[ns]')
OUT:
464 ms ± 28.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
151 µs ± 4.07 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

So we save about 85% of time on a loop, 30% of time on apply, and nothing on astype. My guess is that astype has its own shortcut for when the user tries to cast a Series into its own dtype.
I also hoisted the check to the very top of the constructor and ran this again:

IN:
ts = pd.Timestamp('2020')
%timeit pd.Timestamp(ts)
OUT:
298 ns ± 18 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)


IN:
t_ser = pd.Series(range(100000))
t_ser = t_ser.astype('<M8[ns]')
%timeit t_ser.apply(lambda x: pd.Timestamp(x))
%timeit t_ser.astype('<M8[ns]')
OUT:
451 ms ± 23 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
160 µs ± 8.24 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

So hoisting appears to help on loops.
And also ran it on commit 4c9eb70 (the solution with not any(arg is not None for arg in _non_ts_attributes):

IN:
ts = pd.Timestamp('2020')
%timeit pd.Timestamp(ts)
OUT:
623 ns ± 25.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

IN:
t_ser = pd.Series(range(100000))
t_ser = t_ser.astype('<M8[ns]')
%timeit t_ser.apply(lambda x: pd.Timestamp(x))
%timeit t_ser.astype('<M8[ns]')
OUT:
482 ms ± 18.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
154 µs ± 3.65 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

With list comprehensions, this runs about two times slower in a loop, and a bit slower in lambda. My guess is that since the comprehension doesn't depend on the value converted, it gets optimized away.

Also pinging @jbrockmendel @WillAyd
These testing results should make it easier to decide on what we want to implement here.

jreback

@AlexKirko

ok can you add the benchmarks you did as asv's.

I don't think this is really a big deal as converting scalars is a anti-pattern, we don't do this at scale (sure we do this often on a single basis), but on purpose we don't even store Timestamps (these are stored as i8), so if users are doing it and converting via .apply then tough.

that said this is ok as its not onerous on maitenance.

AlexKirko · 2020-01-18T22:09:19Z

asv_bench/benchmarks/tslibs/timestamp.py

+        Timestamp(self.ts)
+
+    def time_identity_series_apply(self):
+        self.ts_series.apply(lambda x: Timestamp(x))


@jreback

Added benchmarks for scalar cast and apply. Didn't touch astype, as this PR doesn't change astype performance. Is this what you had in mind?

benchmark.tslibs is intended to only depend on _libs.tslibs, so shouldnt have any Series objects in it. just benchmark the Timestamp contructor here

as long as we're at it, might as well have cases for single-arguments for np.datetime64, pydatetime, and tzaware

AlexKirko · 2020-01-20T11:09:04Z

asv_bench/benchmarks/tslibs/timestamp.py

+        Timestamp(self.dttime_aware)
+
+    def time_from_pd_timestamp(self):
+        Timestamp(self.ts)


@jbrockmendel
Was this what you had in mind? I think we should leave only the scalar transformer for benchmarking this shortcut. Don't see much sense in adding the benchmarks for transforming a Series.

this looks good

perfect, thanks

jreback

lgtm. i think if you add @jbrockmendel comment about cython should be good. ping on green.

AlexKirko · 2020-01-21T08:59:16Z

@jreback
Done.

AlexKirko · 2020-01-22T19:19:46Z

@jreback
All green, should be ready to merge.

jreback · 2020-01-24T03:29:30Z

lgtm. can you rebase, ping on green.

AlexKirko · 2020-01-24T09:56:43Z

@jreback
Rebased, all green.
Also cleaned up the conflicts with my previour 2 PRs. Don't need one of the dateutil xfails now.

jreback · 2020-01-26T01:03:45Z

thanks @AlexKirko very nice! keep em coming

…ndexing-1row-df * upstream/master: (194 commits) DOC Remove Python 2 specific comments from documentation (pandas-dev#31198) Follow up PR: pandas-dev#28097 Simplify branch statement (pandas-dev#29243) BUG: DatetimeIndex.snap incorrectly setting freq (pandas-dev#31188) Move DataFrame.info() to live with similar functions (pandas-dev#31317) ENH: accept a dictionary in plot colors (pandas-dev#31071) PERF: add shortcut to Timestamp constructor (pandas-dev#30676) CLN/MAINT: Clean and annotate stata reader and writers (pandas-dev#31072) REF: define _get_slice_axis in correct classes (pandas-dev#31304) BUG: DataFrame.floordiv(ser, axis=0) not matching column-wise bheavior (pandas-dev#31271) PERF: optimize is_scalar, is_iterator (pandas-dev#31294) BUG: Series rolling count ignores min_periods (pandas-dev#30923) xfail sparse warning; closes pandas-dev#31310 (pandas-dev#31311) REF: DatetimeIndex.get_value wrap DTI.get_loc (pandas-dev#31314) CLN: internals.managers (pandas-dev#31316) PERF: avoid copies if possible in fill_binop (pandas-dev#31300) Add test for multiindex json (pandas-dev#31307) BUG: passing TDA and wrong freq to TimedeltaIndex (pandas-dev#31268) BUG: inconsistency between PeriodIndex.get_value vs get_loc (pandas-dev#31172) CLN: remove _set_subtyp (pandas-dev#31301) CI: Updated version of macos image (pandas-dev#31292) ...

jbrockmendel reviewed Jan 4, 2020

View reviewed changes

alimcmaster1 added the Performance Memory or execution speed performance label Jan 4, 2020

AlexKirko force-pushed the perf-timestamp branch from 0a9c79e to ec8249b Compare January 15, 2020 18:00

AlexKirko commented Jan 15, 2020

View reviewed changes

AlexKirko requested a review from jbrockmendel January 15, 2020 20:45

AlexKirko commented Jan 16, 2020

View reviewed changes

AlexKirko mentioned this pull request Jan 16, 2020

PERF: add shortcut to Timedelta constructor #31070

Merged

5 tasks

jreback requested changes Jan 18, 2020

View reviewed changes

jreback added this to the 1.1 milestone Jan 18, 2020

AlexKirko commented Jan 18, 2020

View reviewed changes

AlexKirko requested a review from jreback January 19, 2020 07:30

AlexKirko commented Jan 20, 2020

View reviewed changes

jreback approved these changes Jan 20, 2020

View reviewed changes

AlexKirko requested a review from jreback January 21, 2020 08:59

This was referenced Jan 24, 2020

Performance of maybe_box_datetimelike #30520 #30531

Closed

Performance issue with pandas/core/common.py -> maybe_box_datetimelike #30520

Closed

AlexKirko added 4 commits January 24, 2020 11:00

PERF: add shortcut to Timestamp constructor

6e70d01

CLN: move test to test_constructors.py

9aa0156

BUG: check that only Timestamp is passed

7cebba5

switch to explicit arg is none checks

7aea539

AlexKirko added 6 commits January 24, 2020 11:06

DOC: move whatsnew to version 1.1.0

d2adb4f

TST: add benchmarks for timestamp shortcut

3ea3f91

CLN: run black on the benchmark file

2186358

TST: add scalar benchmarks, remove series benchmark

a195518

fix numpy tzaware datetime constructor call

23f5a44

DOC: comment on reason for verbose check

c80f748

AlexKirko force-pushed the perf-timestamp branch from 33fca04 to c80f748 Compare January 24, 2020 08:07

AlexKirko added 2 commits January 24, 2020 11:52

TST: remove xfail from test_dti_construction_ambiguous_endpoint

9065888

CLN: remove unnecessary LooseVersion import

ba19e26

jreback merged commit 35df212 into pandas-dev:master Jan 26, 2020

AlexKirko deleted the perf-timestamp branch January 27, 2020 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PERF: add shortcut to Timestamp constructor #30676

PERF: add shortcut to Timestamp constructor #30676

AlexKirko commented Jan 4, 2020 •

edited

Loading

AlexKirko commented Jan 4, 2020

jbrockmendel Jan 4, 2020

AlexKirko Jan 9, 2020

AlexKirko Jan 9, 2020 •

edited

Loading

AlexKirko Jan 15, 2020

pep8speaks commented Jan 15, 2020 •

edited

Loading

AlexKirko Jan 15, 2020 •

edited

Loading

AlexKirko Jan 15, 2020

AlexKirko Jan 15, 2020

jbrockmendel Jan 15, 2020

AlexKirko Jan 16, 2020 •

edited

Loading

AlexKirko commented Jan 15, 2020 •

edited

Loading

AlexKirko Jan 16, 2020 •

edited

Loading

WillAyd Jan 16, 2020

jbrockmendel Jan 16, 2020

AlexKirko Jan 17, 2020 •

edited

Loading

jbrockmendel Jan 20, 2020

AlexKirko Jan 21, 2020

jreback commented Jan 17, 2020

AlexKirko commented Jan 18, 2020 •

edited

Loading

jreback left a comment

AlexKirko Jan 18, 2020 •

edited

Loading

jbrockmendel Jan 20, 2020

jbrockmendel Jan 20, 2020

AlexKirko Jan 20, 2020

jreback Jan 20, 2020

jbrockmendel Jan 21, 2020

jreback left a comment

AlexKirko commented Jan 21, 2020

AlexKirko commented Jan 22, 2020

jreback commented Jan 24, 2020

AlexKirko commented Jan 24, 2020

jreback commented Jan 26, 2020

PERF: add shortcut to Timestamp constructor #30676

PERF: add shortcut to Timestamp constructor #30676

Conversation

AlexKirko commented Jan 4, 2020 • edited Loading

AlexKirko commented Jan 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKirko Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jan 15, 2020 • edited Loading

Comment last updated at 2020-01-24 09:24:16 UTC

AlexKirko Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKirko Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlexKirko commented Jan 15, 2020 • edited Loading

AlexKirko Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKirko Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 17, 2020

AlexKirko commented Jan 18, 2020 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

AlexKirko Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

AlexKirko commented Jan 21, 2020

AlexKirko commented Jan 22, 2020

jreback commented Jan 24, 2020

AlexKirko commented Jan 24, 2020

jreback commented Jan 26, 2020

AlexKirko commented Jan 4, 2020 •

edited

Loading

AlexKirko Jan 9, 2020 •

edited

Loading

pep8speaks commented Jan 15, 2020 •

edited

Loading

AlexKirko Jan 15, 2020 •

edited

Loading

AlexKirko Jan 16, 2020 •

edited

Loading

AlexKirko commented Jan 15, 2020 •

edited

Loading

AlexKirko Jan 16, 2020 •

edited

Loading

AlexKirko Jan 17, 2020 •

edited

Loading

AlexKirko commented Jan 18, 2020 •

edited

Loading

AlexKirko Jan 18, 2020 •

edited

Loading