wquantile #117

matthieugomez · 2015-06-10T16:49:32Z

This function implements a weighted quantile. It is a generalization of quantile type 7 as implemented in Julia base (by that I mean results are the same with w = 1).

The function follows the notation in the R help file for quantile, which hopefully will make it easy to add more quantile types. Only non negative weights are allowed.

I've added some tests and documentation.

This is my first pull request so I may be doing some things incorrectly. In particular, I'm not sure about the order of arguments (weight vs quantiles).

johnmyleswhite · 2015-06-10T17:00:33Z

Just to clarify: you didn't read any R source code when implementing this?

nalimilan · 2015-06-10T17:05:02Z

docs/source/scalarstats.rst

@@ -158,6 +158,10 @@ Quantile and Friends
    w = rand(n)
    xk = median(x, weights(w))

+.. function:: wquantile(x, w, q)
+
+  Compute the weighted quantile of ``x``, using weights given by a weight vector ``w`` (of type ``WeightVec``).  Weights must not be negative. The weight and data vectors must have the same length `n`. Quantiles as defined as follow: assume for simplicity that weights sum to `n`. Denote :math:`x_h` the smallest element of ``x`` such that the cumulated sum up of weights up to `x_h` is strictly superior to :math:`lv * q + m`. Denote :math:`x_l` the element just before `x_h`. The sample quantile is defined as `(1-gamma) x_l + gamma x_h`. 


"The weight and data vectors" -> "weights"
"as follow" -> "as follows"
"for simplicity": looks like you can always assume this, right? Better remove these words.
"cumulated sum up of weights up to" -> "cumulative sum of weights up to"
"gamma" -> "\gamma"
Also, looks like several expressions lack :math:.

Oh, and you say "quantile" (singular) while a vector of probabilities is supported. It would make sense to use the plural, and to say a word about the probabilities, following the vocabulary used for ?quantile.

nalimilan · 2015-06-10T17:19:48Z

Thanks! I've only had a look at most obvious things.

Indeed, @johnmyleswhite is right that you must not have taken inspiration from the R code to write this, or it can't be licensed under MIT. It's a bit hard to use their notation without reading their code, though you can always make an implementation from scratch just based on the behaviour described in R docs.

matthieugomez · 2015-06-10T18:40:02Z

Thanks for your comments. I changed my code accordingly. I don't understand what coverage/coveralls is so I'm not sure why it fails.

No I did not look at the R code (which does not compute weighted quantile btw).

A question about your workflow: when I modify a local copy of a package like StatsBase, how can I install this local version as a package? Also, is there a way to load every function in this local version, including non exported function?

nalimilan · 2015-06-10T19:53:24Z

Coveralls is used to measure the portion of the code which is covered by tests. Apparently there are some issues with it at the moment.

When you modify a package in ~/.julia/Package, it's automatically used. You need to checkout the original git branch to use the official version again. I'm not sure how to load all functions from a package; maybe simply by calling include on the toplevel file of the package.

nalimilan · 2015-06-10T19:54:25Z

docs/source/scalarstats.rst

@@ -158,6 +158,9 @@ Quantile and Friends
    w = rand(n)
    xk = median(x, weights(w))

+.. function:: wquantile(x, p, w)


q is p in the code now.

Or rather the contrary.

StefanKarpinski · 2015-06-10T23:07:03Z

@matthieugomez, can you answer the question about whether you read the R source when writing this code? If you did, then I'm afraid that iterating over this patch is a waste of time since the project can't accept it in any case.

johnmyleswhite · 2015-06-10T23:08:09Z

No I did not look at the R code (which does not compute weighted quantile btw).

He did answer it, @StefanKarpinski

nalimilan · 2015-06-11T07:12:26Z

src/weights.jl

+
+
+###### Weighted quantile #####
+# Variables m, g, low, high, correspond respectively to m, gamma, x[j], x[j+1] in the R help file for quantile


FWIW, you can use γ to be closer to the math in Julia.

nalimilan · 2015-06-11T20:21:49Z

src/weights.jl

+                l = 1
+                wthreshold = cumulative_weight - wt[vpermute[i-1]] - index[k] 
+                # loop over posterior elements with same value
+                while (i + l <= lv && v[vpermute[i+l]] == high[k])


The standard Julia style is to put no parentheses after while.

nalimilan · 2015-06-12T08:26:36Z

The tests fail since you rebased, looks like you forgot to export the function.

matthieugomez · 2015-06-12T18:31:28Z

I removed wquantile (instead one should use quantile(v, weight(w))). I also defined quantile for AbstractVector instead of just RealVector but I'm not sure this is correct.

I also used the weighted quantile function to improve the function describe, adding a weighted and a detailed option.

nalimilan · 2015-06-12T22:09:07Z

I'm all for using quantile, but as long as wmedian is exported, it makes sense to export wquantile. Else people might think it's not supported. Regarding the type, I think it's better to keep RealVector, which is used everywhere at the moment. If this needs to be changed, better do that separately for all functions in a different PR.

nalimilan · 2015-06-12T22:09:52Z

src/scalarstats.jl

+
+
+function describe{T<:Real}(a::AbstractArray{T}; detail = false) 
+    detail? show(detailedsummarystats(a)) : show(summarystats(a))


Please add a space before ? (same below).

matthieugomez · 2015-06-12T22:41:29Z

Thanks for the feedback again. What's the best way to use the code for the method of one type to define the method for another type ? Define Abstract type? Write a convert function from weighted to unweighted summary ? I don't see how this would make the code simpler.

On Jun 12, 2015, at 6:09 PM, Milan Bouchet-Valat [email protected] wrote:

I'm all for using quantile, but as long as wmedian is exported, it makes sense to export wquantile. Else people might think it's not supported. Regarding the type, I think it's better to keep RealVector, which is used everywhere at the moment. If this needs to be changed, better do that separately for all functions in a different PR.

—
Reply to this email directly or view it on GitHub.

nalimilan · 2015-06-13T08:00:34Z

I think you can define an abstract type, say AbstractSummaryStats, from which both types inherit. Then the unweighted version will apply to AbstractSummaryStats, so that you can call it without any conversion from the weighted version.

nalimilan · 2015-06-13T08:04:29Z

Even simpler: you could merge all (non-)detailed and (un-)weighted printing methods into one method parameterized on T <: AbstractSummaryStats, and just do if T == WeightedSummaryStats before the line printing the weights and if T == DetailedSummaryStats before the lines printing detailed stats.

matthieugomez · 2015-08-20T15:37:53Z

I've simplified the wquantile code and added tests for the describe method.
What should I do to have this merged?

andreasnoack · 2015-08-20T16:09:06Z

You'll have to rebase. @nalimilan You've done most of the review, is this ready for merging?

aviks · 2015-09-16T08:15:30Z

Bump?

matthieugomez · 2015-10-02T17:36:19Z

It's been a while. Can this be merged? I've suppressed the types to display weighted summary stats and I'll write it as a different pull request.

johnmyleswhite · 2015-10-05T15:38:00Z

This looks good to merge. What's going on with Travis?

andreasnoack · 2015-10-05T15:49:54Z

There have been some certificate problems on the nightlies, but they should vbe fixed now. I've restarted the build and it seems to work.

wquantile

johnmyleswhite · 2015-10-06T22:00:14Z

Thanks for working on this, @matthieugomez.

nalimilan reviewed Jun 10, 2015
View reviewed changes

matthieugomez closed this Jun 10, 2015

matthieugomez reopened this Jun 10, 2015

nalimilan reviewed Jun 10, 2015
View reviewed changes

nalimilan reviewed Jun 11, 2015
View reviewed changes

matthieugomez closed this Jun 11, 2015

matthieugomez reopened this Jun 11, 2015

nalimilan reviewed Jun 11, 2015
View reviewed changes

nalimilan reviewed Jun 12, 2015
View reviewed changes

matthieugomez closed this Oct 2, 2015

matthieugomez reopened this Oct 2, 2015

wquantile

710eb38

johnmyleswhite added a commit that referenced this pull request Oct 6, 2015

Merge pull request #117 from matthieugomez/master

7397c85

wquantile

johnmyleswhite merged commit 7397c85 into JuliaStats:master Oct 6, 2015

ararslan mentioned this pull request Jun 25, 2016

Feature request: weighted quantiles #100

Closed

nalimilan mentioned this pull request Nov 7, 2017

Quantiles don't respect weights of 0 #313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wquantile #117

wquantile #117

matthieugomez commented Jun 10, 2015

johnmyleswhite commented Jun 10, 2015

nalimilan Jun 10, 2015

nalimilan Jun 10, 2015

nalimilan commented Jun 10, 2015

matthieugomez commented Jun 10, 2015

nalimilan commented Jun 10, 2015

nalimilan Jun 10, 2015

nalimilan Jun 10, 2015

StefanKarpinski commented Jun 10, 2015

johnmyleswhite commented Jun 10, 2015

nalimilan Jun 11, 2015

nalimilan Jun 11, 2015

nalimilan commented Jun 12, 2015

matthieugomez commented Jun 12, 2015

nalimilan commented Jun 12, 2015

nalimilan Jun 12, 2015

matthieugomez commented Jun 12, 2015

nalimilan commented Jun 13, 2015

nalimilan commented Jun 13, 2015

matthieugomez commented Aug 20, 2015

andreasnoack commented Aug 20, 2015

aviks commented Sep 16, 2015

matthieugomez commented Oct 2, 2015

johnmyleswhite commented Oct 5, 2015

andreasnoack commented Oct 5, 2015

johnmyleswhite commented Oct 6, 2015



		###### Weighted quantile #####
		# Variables m, g, low, high, correspond respectively to m, gamma, x[j], x[j+1] in the R help file for quantile



		function describe{T<:Real}(a::AbstractArray{T}; detail = false)
		detail? show(detailedsummarystats(a)) : show(summarystats(a))

wquantile #117

wquantile #117

Conversation

matthieugomez commented Jun 10, 2015

johnmyleswhite commented Jun 10, 2015

nalimilan Jun 10, 2015

Choose a reason for hiding this comment

nalimilan Jun 10, 2015

Choose a reason for hiding this comment

nalimilan commented Jun 10, 2015

matthieugomez commented Jun 10, 2015

nalimilan commented Jun 10, 2015

nalimilan Jun 10, 2015

Choose a reason for hiding this comment

nalimilan Jun 10, 2015

Choose a reason for hiding this comment

StefanKarpinski commented Jun 10, 2015

johnmyleswhite commented Jun 10, 2015

nalimilan Jun 11, 2015

Choose a reason for hiding this comment

nalimilan Jun 11, 2015

Choose a reason for hiding this comment

nalimilan commented Jun 12, 2015

matthieugomez commented Jun 12, 2015

nalimilan commented Jun 12, 2015

nalimilan Jun 12, 2015

Choose a reason for hiding this comment

matthieugomez commented Jun 12, 2015

nalimilan commented Jun 13, 2015

nalimilan commented Jun 13, 2015

matthieugomez commented Aug 20, 2015

andreasnoack commented Aug 20, 2015

aviks commented Sep 16, 2015

matthieugomez commented Oct 2, 2015

johnmyleswhite commented Oct 5, 2015

andreasnoack commented Oct 5, 2015

johnmyleswhite commented Oct 6, 2015