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: performance regression in Series.asof #14476

Merged
merged 3 commits into from
Oct 26, 2016

Conversation

laudney
Copy link
Contributor

@laudney laudney commented Oct 22, 2016

Fix performance regression in Series.asof by avoiding pre-computing nulls and returning value by indexing the underlying ndarray.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

needs an asv on several different cases
this will be very slow with lots of nulls

@laudney
Copy link
Contributor Author

laudney commented Oct 22, 2016

@jreback you mean we should always pre-compute nulls for DataFrame?

@jreback
Copy link
Contributor

jreback commented Oct 22, 2016

the answer is probably yes
but you need some perf testing in several scenarios to see that is an asv

@codecov-io
Copy link

codecov-io commented Oct 22, 2016

Current coverage is 85.26% (diff: 100%)

Merging #14476 into master will increase coverage by <.01%

@@             master     #14476   diff @@
==========================================
  Files           140        140          
  Lines         50667      50670     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43203      43206     +3   
  Misses         7464       7464          
  Partials          0          0          

Powered by Codecov. Last update d1d75d7...2e4daba

@laudney
Copy link
Contributor Author

laudney commented Oct 22, 2016

@jreback what's asv?

@jreback
Copy link
Contributor

jreback commented Oct 22, 2016

@jorisvandenbossche
Copy link
Member

There are some benchmarks for the series case already: https://github.com/pandas-dev/pandas/blob/master/asv_bench/benchmarks/timeseries.py#L283. We should indeed add for DataFrame.

For Series I suspect that the approach in this PR will also be faster with lots of NaNs, as the indexing of Series is much slower that the isnull checking on single values. For DataFrame not directly sure, so there is may make sense to precompute the NaNs (should be checked).

@jorisvandenbossche jorisvandenbossche added the Performance Memory or execution speed performance label Oct 22, 2016
@jorisvandenbossche jorisvandenbossche changed the title Fix performance regression in Series.asof by avoiding pre-computing n… PERF: performance regression in Series.asof Oct 22, 2016
@laudney
Copy link
Contributor Author

laudney commented Oct 22, 2016

asv ... -b ^timeseries

    before     after       ratio
  [233d51dd] [4836cdfd]
+  232.75μs   263.67μs      1.13  timeseries.timeseries_custom_bmonthend_incr.time_timeseries_custom_bmonthend_incr
+   14.06ms    15.68ms      1.11  timeseries.datetime_algorithm.time_dti_factorize
+   24.23μs    26.87μs      1.11  timeseries.timeseries_year_apply.time_timeseries_year_apply
+  338.21μs   374.77μs      1.11  timeseries.timeseries_custom_bmonthend_decr_n.time_timeseries_custom_bmonthend_decr_n
+  687.57ms   761.01ms      1.11  timeseries.timeseries_semi_month_offset.time_semi_month_begin_decr_rng
-   27.32ms    24.70ms      0.90  timeseries.timeseries_iter_periodindex_preexit.time_timeseries_iter_periodindex_preexit
-    3.12ms     2.77ms      0.89  timeseries.dataframe_resample_mean_numpy.time_dataframe_resample_mean_numpy
-    3.45ms     2.99ms      0.87  timeseries.dataframe_resample_max_string.time_dataframe_resample_max_string
-  314.65μs   271.87μs      0.86  timeseries.datetimeindex_add_offset.time_datetimeindex_add_offset
-  409.15μs    24.12μs      0.06  timeseries.timeseries_asof_single.time_timeseries_asof_single

@jorisvandenbossche
Copy link
Member

@laudney You can selectively run only the asof ones with -b asof (in this case that is enough, as you do not touch code related to other functionality).

@laudney
Copy link
Contributor Author

laudney commented Oct 22, 2016

asv ... -b asof

    before     after       ratio
  [233d51dd] [4836cdfd]
-  454.34μs    22.44μs      0.05  timeseries.timeseries_asof_single.time_timeseries_asof_single

@jreback
Copy link
Contributor

jreback commented Oct 22, 2016

need several more asvs

goal_time = 0.2

def setup(self):
self.N = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

these are too small make 100000

put nans at beginning and another one at the end

@laudney
Copy link
Contributor Author

laudney commented Oct 24, 2016

I've updated my pull request code to refactor existing asv tests for Series.asof and add new ones for DataFrame.asof.

The main findings are:

  1. Avoid pre-computation of nulls for Series if where is a scalar because the while loop is always faster than the pre-computation.
  2. Always pre-compute nulls for DataFrames. The while loop is extremely expensive. Given pre-computation is always available, even if where is a scalar, should avoid the while loop and use the same code path as when where is list-like.

asv results below:

    before     after       ratio
  [233d51dd] [4cd64641]
+   15.63ms    20.19ms      1.29  timeseries.timeseries_dataframe_asof.time_asof_single
-  429.14μs    22.11μs      0.05  timeseries.timeseries_asof.time_asof_single
-     1.28s    21.10ms      0.02  timeseries.timeseries_dataframe_asof.time_asof_nan_single
-     1.24s     3.56ms      0.00  timeseries.timeseries_asof.time_asof_nan_single

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can u add your explanation of perf in a comment
with a ref to this pr number in a comment as well

lgtm otherwise

@laudney
Copy link
Contributor Author

laudney commented Oct 24, 2016

@jreback I've added comment in the PR. Please check and let me know. Off to bed.

@laudney
Copy link
Contributor Author

laudney commented Oct 25, 2016

I will add two more asv tests and more detailed comment tonight. I will ping you when ready.

@jorisvandenbossche jorisvandenbossche added this to the 0.19.1 milestone Oct 25, 2016
@jreback
Copy link
Contributor

jreback commented Oct 25, 2016

lgtm. pls add a whatsnew entry in the performance section. ping on green.

…ulls and returning value by indexing the underlying ndarray.
@laudney
Copy link
Contributor Author

laudney commented Oct 25, 2016

@jreback @jorisvandenbossche I've updated the PR: more comment and more asv tests. It's ready. Please review and merge.

impact as below:

-   87.33μs    18.44μs      0.21  timeseries.timeseries_asof.time_asof_single_early
-  390.14μs    21.42μs      0.05  timeseries.timeseries_asof.time_asof_single
-     1.20s     3.34ms      0.00  timeseries.timeseries_asof.time_asof_nan_single
-     1.26s    21.55ms      0.02  timeseries.timeseries_dataframe_asof.time_asof_nan_single
-   14.65ms    20.11μs      0.00  timeseries.timeseries_dataframe_asof.time_asof_single_early

@jreback
Copy link
Contributor

jreback commented Oct 25, 2016

@laudney pls add a whatsnew note

@laudney
Copy link
Contributor Author

laudney commented Oct 25, 2016

@jreback whatsnew updated for 0.19.1

@laudney
Copy link
Contributor Author

laudney commented Oct 26, 2016

@jreback @jorisvandenbossche It's all green on my side. Let me know if you need anything else.

@jorisvandenbossche jorisvandenbossche merged commit e3d943d into pandas-dev:master Oct 26, 2016
@jorisvandenbossche
Copy link
Member

@laudney Thanks!

jorisvandenbossche pushed a commit to jorisvandenbossche/pandas that referenced this pull request Nov 2, 2016
…of (pandas-dev#14476)

* Fix performance regression in Series.asof by avoiding pre-computing nulls and returning value by indexing the underlying ndarray.
(cherry picked from commit e3d943d)
yarikoptic added a commit to neurodebian/pandas that referenced this pull request Nov 18, 2016
Version 0.19.1

* tag 'v0.19.1': (43 commits)
  RLS: v0.19.1
  DOC: update whatsnew/release notes for 0.19.1 (pandas-dev#14573)
  [Backport pandas-dev#14545] BUG/API: Index.append with mixed object/Categorical indices (pandas-dev#14545)
  DOC: rst fixes
  [Backport pandas-dev#14567] DEPR: add deprecation warning for com.array_equivalent (pandas-dev#14567)
  [Backport pandas-dev#14551] PERF: casting loc to labels dtype before searchsorted (pandas-dev#14551)
  [Backport pandas-dev#14536] BUG: DataFrame.quantile with NaNs (GH14357) (pandas-dev#14536)
  [Backport pandas-dev#14520] BUG: don't close user-provided file handles in C parser (GH14418) (pandas-dev#14520)
  [Backport pandas-dev#14392] BUG: Dataframe constructor when given dict with None value (pandas-dev#14392)
  [Backport pandas-dev#14514] BUG: Don't parse inline quotes in skipped lines (pandas-dev#14514)
  [Bacport pandas-dev#14543] BUG: tseries ceil doc fix (pandas-dev#14543)
  [Backport pandas-dev#14541] DOC: Simplify the gbq integration testing procedure for contributors (pandas-dev#14541)
  [Backport pandas-dev#14527] BUG/ERR: raise correct error when sql driver is not installed (pandas-dev#14527)
  [Backport pandas-dev#14501] BUG: fix DatetimeIndex._maybe_cast_slice_bound for empty index (GH14354) (pandas-dev#14501)
  [Backport pandas-dev#14442] DOC: Expand on reference docs for read_json() (pandas-dev#14442)
  BLD: fix 3.4 build for cython to 0.24.1
  [Backport pandas-dev#14492] BUG: Accept unicode quotechars again in pd.read_csv
  [Backport pandas-dev#14496] BLD: Support Cython 0.25
  [Backport pandas-dev#14498] COMPAT/TST: fix test for range testing of negative integers to neg powers
  [Backport pandas-dev#14476] PERF: performance regression in Series.asof (pandas-dev#14476)
  ...
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.

pd.Series.asof performance regression: 14x slower
4 participants