Skip to content
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

PERF: Fix regression in datetime ops #17980

Merged
merged 3 commits into from
Oct 26, 2017

Conversation

TomAugspurger
Copy link
Contributor

HEAD:

In [1]: import pandas as pd; import numpy as np
In [2]: s = pd.Series(pd.to_datetime(np.arange(100000), unit='ms'))
In [3]: %timeit s - s.shift()
2.73 ms ± 30.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

0.21.0rc1:

527 ms ± 11.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

0.20.3

2.4 ms ± 57.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

xref #17861

HEAD:

```
In [1]: import pandas as pd; import numpy as np
In [2]: s = pd.Series(pd.to_datetime(np.arange(100000), unit='ms'))
In [3]: %timeit s - s.shift()
2.73 ms ± 30.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

0.21.0rc1:

```
527 ms ± 11.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
```

0.20.3

```
2.4 ms ± 57.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```
@TomAugspurger
Copy link
Contributor Author

This also seems to help with the timeseries.SeriesArithmetic.time_add_offset benchmarks:

head:

[ 33.33%] ··· Running timeseries.SeriesArithmetic.time_add_offset_delta                                                                               4.30ms
[ 66.67%] ··· Running timeseries.SeriesArithmetic.time_add_offset_fast                                                                                11.0ms
[100.00%] ··· Running timeseries.SeriesArithmetic.time_add_offset_slow                                                                                 703ms

master:

[ 33.33%] ··· Running timeseries.SeriesArithmetic.time_add_offset_delta                                                                                499ms
[ 66.67%] ··· Running timeseries.SeriesArithmetic.time_add_offset_fast                                                                                 489ms
[100.00%] ··· Running timeseries.SeriesArithmetic.time_add_offset_slow                                                                                 1.05s

@TomAugspurger TomAugspurger added this to the 0.21.0 milestone Oct 25, 2017
@TomAugspurger TomAugspurger added the Performance Memory or execution speed performance label Oct 25, 2017
@codecov
Copy link

codecov bot commented Oct 25, 2017

Codecov Report

Merging #17980 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17980      +/-   ##
==========================================
- Coverage   91.23%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       50113    50113              
==========================================
- Hits        45723    45714       -9     
- Misses       4390     4399       +9
Flag Coverage Δ
#multiple 89.03% <100%> (ø) ⬆️
#single 40.31% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/ops.py 91.77% <100%> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.75% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36c309e...d0ea5dc. Read the comment docs.

@TomAugspurger
Copy link
Contributor Author

Cool, this also fixes the slowdown in the stata benchmarks:

head:

[  0.00%] ·· Building for existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3.6
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3.6
[ 25.00%] ··· Running packers.STATA.time_write_stata                                                                                                  33.2ms
[ 50.00%] ··· Running packers.STATA.time_write_stata_with_validation                                                                                  49.5ms
[ 75.00%] ··· Running packers.packers_read_stata.time_packers_read_stata                                                                              39.6ms
[100.00%] ··· Running packers.packers_read_stata_with_validation.time_packers_read_stata_with_validation                                              48.0ms

master:

[  0.00%] ·· Building for existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3.6
[  0.00%] ·· Benchmarking existing-py_Users_taugspurger_Envs_pandas-dev_bin_python3.6
[ 25.00%] ··· Running packers.STATA.time_write_stata                                                                                                   502ms
[ 50.00%] ··· Running packers.STATA.time_write_stata_with_validation                                                                                   529ms
[ 75.00%] ··· Running packers.packers_read_stata.time_packers_read_stata                                                                               631ms
[100.00%] ··· Running packers.packers_read_stata_with_validation.time_packers_read_stata_with_validation                                               650ms

@jbrockmendel
Copy link
Member

Are the isinstance checks being called zillions of times? ABCDateOffset was introduced recently, and isinstance(x, ABCFoo) is about 2x slower than isinstance(x, Foo)

@TomAugspurger
Copy link
Contributor Author

Are the isinstance checks being called zillions of times?

On master they were being called once per row. The change here skips that, since if we have a datetime64 or timedelta64 array, then we know none of them are offsets.

@@ -622,6 +622,10 @@ def _is_offset(self, arr_or_obj):
""" check if obj or all elements of list-like is DateOffset """
if isinstance(arr_or_obj, ABCDateOffset):
return True
elif (is_datetime64_dtype(arr_or_obj) or
is_timedelta64_dtype(arr_or_obj)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catch period here as well?

Copy link
Contributor Author

@TomAugspurger TomAugspurger Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What binary ops is a PeriodIndex valid in (if any)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_period

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, we have is_period_dtype which is the correct to use here

elif (is_datetime64_dtype(arr_or_obj) or
is_timedelta64_dtype(arr_or_obj)):
# Don't want to check elementwise for Series / array of datetime
return False
elif is_list_like(arr_or_obj) and len(arr_or_obj):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you actually only do this check if is_object_dtype is True in the first place, otherwise no point in iterating at all (if its not a scalar ABCDateOffset)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think d0ea5dc has want you meant, much cleaner.

@TomAugspurger TomAugspurger merged commit 6779ac0 into pandas-dev:master Oct 26, 2017
@TomAugspurger TomAugspurger deleted the dtype-infer branch October 26, 2017 13:57
@jorisvandenbossche
Copy link
Member

Thanks for hunting those down!

peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request Oct 31, 2017
* PERF: Fix regression in datetime ops

HEAD:

```
In [1]: import pandas as pd; import numpy as np
In [2]: s = pd.Series(pd.to_datetime(np.arange(100000), unit='ms'))
In [3]: %timeit s - s.shift()
2.73 ms ± 30.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

0.21.0rc1:

```
527 ms ± 11.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
```

0.20.3

```
2.4 ms ± 57.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

* timedelta too

* Clean up the fix
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
* PERF: Fix regression in datetime ops

HEAD:

```
In [1]: import pandas as pd; import numpy as np
In [2]: s = pd.Series(pd.to_datetime(np.arange(100000), unit='ms'))
In [3]: %timeit s - s.shift()
2.73 ms ± 30.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

0.21.0rc1:

```
527 ms ± 11.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
```

0.20.3

```
2.4 ms ± 57.8 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

* timedelta too

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

Successfully merging this pull request may close these issues.

4 participants