improve quantile: reduce allocations, use partial sort #14413
Conversation
|
Potential behavior changes like this are why I generally don't backport anything until I get a chance to run PackageEvaluator with the change included. |
|
There should also be tests of any corner cases, if there aren't yet. |
|
👍 |
|
Good point, I'll add some more tests. By the way, I forgot to mention that this add |
|
This performance improvement may be worth capturing in NEWS.md. |
| """ | ||
| quantile!([q, ] v, p; sorted=false) | ||
|
|
||
| Compute the quantile(s) of a vector `v` at the probabilities `p`, with optional output into array `q` (if not provided, a new output array is created). The keyword argument `sorted` indicates whether `v` can be assumed to be sorted; if `false` (the default), then the elements of `v` may be partially sorted. |
There was a problem hiding this comment.
line breaks
and should be sure the rst sig is consistent, and rerun genstdlib
There was a problem hiding this comment.
Ah, right. I haven't edited the docs since the a-doc-alypse.
simonbyrne
force-pushed
the
sb/quantile
branch
2 times, most recently
from
ff97cdd
to
7bd187f
Compare
|
Okay, rebased. Hopefully this should all be good to go. |
| function quantile!(v::AbstractVector, q::AbstractVector) | ||
| isempty(v) && throw(ArgumentError("empty data array")) | ||
| isempty(q) && throw(ArgumentError("empty quantile array")) | ||
| doc""" |
There was a problem hiding this comment.
#14378 moved almost completely away from doc""". Latex is now denoted by double backticks.
There was a problem hiding this comment.
Ah, good to know. Fixed.
simonbyrne
force-pushed
the
sb/quantile
branch
from
7bd187f
to
f6ad90e
Compare
improve quantile: reduce allocations, use partial sort
|
|
||
| function bound_quantiles(qs::AbstractVector) |
There was a problem hiding this comment.
hopefully no one was using this? only public code on github I can find that isn't a julia fork is StatsBase's own copy, so should be okay
There was a problem hiding this comment.
It wasn't exported, so I think it is safe to remove.
There was a problem hiding this comment.
on master yes, but not necessarily for a backport
This improves the performance of
quantileby reducing allocations and using a partial sort. I see a 3x improvement for large cases.It should be mostly non-breaking, but it does slightly change the behaviour of values outside the interval
[0,1](the previous version was a bit more lax), but I don't think this should prevent backporting.