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

Improve quantile performance #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

bkamins
Copy link
Contributor

@bkamins bkamins commented Sep 8, 2021

Fixes #84

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #86 (f8015c8) into master (54f9b0d) will decrease coverage by 1.96%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   98.18%   96.22%   -1.97%     
==========================================
  Files           1        1              
  Lines         386      424      +38     
==========================================
+ Hits          379      408      +29     
- Misses          7       16       +9     
Impacted Files Coverage Δ
src/Statistics.jl 96.22% <75.00%> (-1.97%) ⬇️

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 54f9b0d...f8015c8. Read the comment docs.

@bkamins
Copy link
Contributor Author

bkamins commented Sep 8, 2021

I still need to add correctness tests.

@bkamins
Copy link
Contributor Author

bkamins commented Sep 8, 2021

@nalimilan - doing an "additional sorting" is not correct unfortunately (the second sorting destroys the first), so we need to do this independently unfortunately (still it should not be that bad).

src/Statistics.jl Outdated Show resolved Hide resolved
src/Statistics.jl Outdated Show resolved Hide resolved
src/Statistics.jl Outdated Show resolved Hide resolved
src/Statistics.jl Outdated Show resolved Hide resolved
src/Statistics.jl Outdated Show resolved Hide resolved
@@ -937,22 +937,33 @@ function quantile!(q::AbstractArray, v::AbstractVector, p::AbstractArray;
end
isempty(q) && return q

minp, maxp = extrema(p)
_quantilesort!(v, sorted, minp, maxp)
if length(p) == 2
Copy link
Member

Choose a reason for hiding this comment

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

Special-casing 2 is kind of weird. For example, for [0.25, 0.5, 0.75] this branch would probably also be faster, right? Actually, isn't this approach faster than the other in most cases?

A possible optimization would be to call partialsort! on the full array for the first quantile, then call it on a view from the first quantile to the end of the array for the second, and so on. Not sure that would make a big difference, but at least it shouldn't be really slower than sorting everything between the two extreme quantiles, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is faster:

julia> using Statistics

julia> using BenchmarkTools

julia> x = rand(10^4);

julia> @btime quantile($x, [0.25, 0.5, 0.75]);
  410.400 μs (4 allocations: 78.42 KiB)

julia> @btime [quantile($x, 0.25), quantile($x, 0.5), quantile($x, 0.75)];
  314.000 μs (7 allocations: 234.72 KiB)

The problem is that quantiles do not need to be sorted, so this complicates the code (but of course is doable as I guess sorting requested quantiles should not be problematic for performance).

We do not need views I think, as sort! supports passing start and stop ranges.

However, as it seems to be a bigger rework I will leave it for after DataFrames.jl 1.3 releaes when we work on general Statistics update.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Yes, sorting quantiles should have a negligible cost (and most of the time they will already be sorted).

Copy link
Member

Choose a reason for hiding this comment

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

Do you feel like finishing this?

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 have opened https://github.com/JuliaLang/Statistics.jl/pull/91 to perform easy comparison of both.

@nalimilan nalimilan changed the title Improve quicksort performance Improve quantile performance Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quantile is inefficient in common case of CI calculation
2 participants