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

Performance regression in rolling.Quantile.time_quantile #33913

Closed
TomAugspurger opened this issue May 1, 2020 · 9 comments
Closed

Performance regression in rolling.Quantile.time_quantile #33913

TomAugspurger opened this issue May 1, 2020 · 9 comments
Labels
Performance Memory or execution speed performance quantile quantile method Regression Functionality that used to work in a prior pandas version

Comments

@TomAugspurger
Copy link
Contributor

TomAugspurger commented May 1, 2020

https://pandas.pydata.org/speed/pandas/index.html#rolling.Quantile.time_quantile?p-constructor=%27Series%27&p-window=1000&p-dtype=%27int%27&p-percentile=1&p-param5=%27linear%27&commits=df5eee60d60a604969bdc3e36a0da20ea36af3f6.

import pandas as pd
import numpy as np

constructor = "Series"
window = 1000
dtype = "int"
percentile = 1
interpolation = "linear"


N = 10 ** 5
arr = np.random.random(N).astype(dtype)
roll = getattr(pd, constructor)(arr).rolling(window)

%timeit roll.quantile(percentile, interpolation=interpolation)

Seems to be from #33693 (cc @s-scherrer).

Interestingly, I'm not able to reproduce this locally. Is anyone else able to?

# 1.0.2
3.16 ms ± 295 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

# master
2.27 ms ± 56.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
@TomAugspurger TomAugspurger added Bug Needs Triage Issue that has not been reviewed by a pandas team member Performance Memory or execution speed performance Regression Functionality that used to work in a prior pandas version and removed Bug Needs Triage Issue that has not been reviewed by a pandas team member labels May 1, 2020
@TomAugspurger
Copy link
Contributor Author

May be more reproducible with smaller window sizes (window=10).

@jreback
Copy link
Contributor

jreback commented May 1, 2020

how is this a regression unless your numbers are backwards?

@s-scherrer
Copy link
Contributor

Interesting. I only ran the memory benchmarks, but when reviewing my results (posted in #33693), the times were sometimes a bit higher after my changes, except in the last run. In general the times were often quite different between runs, so I thought this was due to background processes on my system.

The only reason I could think of is that I changed the functions to pass memoryviews (float64_t[:]) instead of ndarray[float64_t], because I thought they were in general faster.

@s-scherrer
Copy link
Contributor

s-scherrer commented May 2, 2020

I could reproduce the behaviour on my machine and did some tests, but the results are a bit strange.

As I mentioned in my PR (#33693), there were two ways to fix the memory problem:

  • fix 1: remove the unused arguments starti and endi from the function signature of _roll_min_max_fixed
  • fix 2: pass memoryviews instead of arrays in roll_min_fixed, roll_max_fixed, and _roll_min_max_fixed.

I implemented both fixes in #33639. In the comment above, I assumed that fix 2 might have been the problem. Therefore, I ran the Quantile benchmarks with different versions of aggregations.pyx:

  • master before my PR (master)
  • last commit of my PR (fix 1 & 2)
  • changed memoryviews back to arrays (fix 1)
  • all changes in aggregations.pyx reverted to like they where on master (the paranoid case, like master)
  • passing only memoryviews (fix 2)

Shown are just the results for the benchmark from above, (Series, 10, int, 1):

master:
Series, 10, int, 1: 8.49 +- 0.04 ms
Series, 10, int, 1: 8.39 +- 0.05 ms

fix 1 & 2:
Series, 10, int, 1: 11.9 +- 0.03 ms
Series, 10, int, 1: 11.9 +- 0.07 ms

fix 1:
Series, 10, int, 1: 12.2 +- 0.2 ms
Series, 10, int, 1: 12.0 +- 0.08 ms

like master:
Series, 10, int, 1: 8.43 +- 0.03 ms
Series, 10, int, 1: 8.60 +- 0.02 ms

fix 2:
Series, 10, int, 1: 12.0 +- 0.02 ms
Series, 10, int, 1: 12.0 +- 0.05 ms

As can be seen, the "fixed" versions run about 40% longer than master. The results are consistent with what @TomAugspurger showed above.
However, I am completely lost when it comes to interpreting this. I would assume that fix 1 (removing the unused argument) shouldn't have an impact at all, or at least a positive impact.

@simonjayhawkins simonjayhawkins modified the milestones: 1.1, 1.0.4 May 16, 2020
@TomAugspurger TomAugspurger modified the milestones: 1.0.4, 1.1 May 29, 2020
@jreback jreback modified the milestones: 1.1, 1.1.1 Jul 10, 2020
@simonjayhawkins simonjayhawkins modified the milestones: 1.1.1, 1.1.2 Aug 17, 2020
@simonjayhawkins
Copy link
Member

moved off 1.1.1 milestone (scheduled for this week) as no PRs to fix in the pipeline

@simonjayhawkins simonjayhawkins modified the milestones: 1.1.2, 1.1.3 Sep 7, 2020
@simonjayhawkins
Copy link
Member

moved off 1.1.2 milestone (scheduled for this week) as no PRs to fix in the pipeline

@simonjayhawkins simonjayhawkins modified the milestones: 1.1.3, 1.1.4 Oct 5, 2020
@simonjayhawkins
Copy link
Member

moved off 1.1.3 milestone (overdue) as no PRs to fix in the pipeline

@simonjayhawkins
Copy link
Member

moved off 1.1.4 milestone (scheduled for release tomorrow) as no PRs to fix in the pipeline

@simonjayhawkins simonjayhawkins modified the milestones: 1.1.4, 1.1.5 Oct 29, 2020
@jreback jreback removed this from the 1.1.5 milestone Nov 25, 2020
@jreback jreback added this to the Contributions Welcome milestone Nov 25, 2020
@mroeschke mroeschke added the quantile quantile method label Aug 7, 2021
@mroeschke mroeschke removed this from the Contributions Welcome milestone Oct 13, 2022
@mroeschke
Copy link
Member

Given this issue is with an unsupported version of pandas now, going to close.

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 quantile quantile method Regression Functionality that used to work in a prior pandas version
Projects
None yet
Development

No branches or pull requests

5 participants