Implement quantile binning #34
Conversation
| n ≥ nbins || error("too many bins requested") | ||
|
|
||
| qs = range(0, 1; length=(nbins+1)) | ||
| retval = quantile.(Ref(data), qs) |
There was a problem hiding this comment.
I think retval = quantile(data, qs) might be better. If we broadcast, every call probably has to sort data. If we pass in one list and ask Statistics.pkg to give us the quantiles, it can properly reason about things under the hood to be more efficient.
| qs = range(0, 1; length=(nbins+1)) | ||
| retval = quantile.(Ref(data), qs) | ||
|
|
||
| isunique = diff(retval) .> 1e-8 |
There was a problem hiding this comment.
This is the opposite of is-unique, but rather whether the bin edge is different. Can we do:
# trim bins that are too small
keep_bin = diff(retval) .> alg.max_bin_width
| @@ -0,0 +1,15 @@ | |||
| @test DiscretizeQuantile(2) == DiscretizeQuantile(2, false) | |||
There was a problem hiding this comment.
Thank you for adding tests!
It may not be covered by the other methods, but could you please test [1,1,1] and verify that we throw an error?
| mask = vcat([true], isunique) | ||
| retval = retval[mask] # note this makes length(retval) < nbins | ||
| else | ||
| # Throw error as in other methods if there are non-unique bin egdes |
|
|
||
| if alg.trim # trim bins that are too small | ||
| # Ref: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/preprocessing/_discretization.py#L288 | ||
| mask = vcat([true], isunique) |
There was a problem hiding this comment.
Could we do:
# Always keep the first bin edge
insert!(keep_bin, 1, true)
retval = retgval[keep_bin] # NOTE: this reduces our bin count
and then ensure that we have at least 2 edges remaining?
|
|
||
| struct DiscretizeQuantile <: DiscretizationAlgorithm | ||
| nbins::Int | ||
| trim::Bool |
There was a problem hiding this comment.
Could we add a simple comment?
trim::Bool # Whether to automatically remove extremely narrow bins
| retval = retval[mask] # note this makes length(retval) < nbins | ||
| else | ||
| # Throw error as in other methods if there are non-unique bin egdes | ||
| any(.!(isunique)) && error("binedges non-unique") |
There was a problem hiding this comment.
I think any(.!(isunique)) allocates a new vector. How about !all(keep_bin)?
| @@ -0,0 +1,32 @@ | |||
| using Statistics: quantile | |||
There was a problem hiding this comment.
Could this be added at the top-level, in Discretizers.jl?
| @@ -20,6 +20,7 @@ export | |||
| # discretization algorithms | |||
| DiscretizeUniformWidth, | |||
| DiscretizeUniformCount, | |||
| DiscretizeQuantile, | |||
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 93.95% 94.05% +0.10%
==========================================
Files 9 10 +1
Lines 877 892 +15
==========================================
+ Hits 824 839 +15
Misses 53 53
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Following #33 we decide to implement a separate binning based on the quantile directly.