Change the definition of `wmedian` to `wquantile(., 0.5)` #436

matthieugomez · 2018-12-09T23:03:34Z

and returns an error for non integer FrequencyWeights.
See #435
I wish there was a NEWS section to document the change, because wmedian now gives a different result.

codecov · 2018-12-10T00:37:01Z

Codecov Report

Merging #436 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   85.09%   84.97%   -0.12%     
==========================================
  Files          18       18              
  Lines        1992     1963      -29     
==========================================
- Hits         1695     1668      -27     
+ Misses        297      295       -2

Impacted Files	Coverage Δ
src/weights.jl	`78.94% <100%> (-1.88%)`	⬇️

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 2913b7a...c66f39d. Read the comment docs.

codecov · 2018-12-10T00:37:01Z

Codecov Report

Merging #436 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   85.06%   84.95%   -0.12%     
==========================================
  Files          21       21              
  Lines        2116     2080      -36     
==========================================
- Hits         1800     1767      -33     
+ Misses        316      313       -3

Impacted Files	Coverage Δ
src/deprecates.jl	`0% <ø> (ø)`	⬆️
src/weights.jl	`78.19% <100%> (-2.17%)`	⬇️

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 6ce5225...f09e1f8. Read the comment docs.

put median after quantile

nalimilan · 2019-02-01T18:53:41Z

We should revive this PR. Sorry for the delay.

For future reference, can you explain why median differs from quantile currently? What definition does it implement? @andreasnoack thinks there aren't so many different definitions of the weighted median as there are for quantiles, but I'm not sure that's the case.

matthieugomez · 2019-02-01T19:12:09Z

median currently is:

take the maximum value such that sum of weights of values below the value is lower or equal than 1/2
take the minimum value such that sum of weights of values above is lower or equal than 1/2.
If these are the same values, you've found the median. Otherwise, take the average.

It corresponds to a nice extension of the median definition from unweighted to weighted vectors. The issue is this generalization does not help when generalizing other quantiles, so I think is better to use a generalization that is defined for any quantile.

The other issue with the current implementation is that, with frequency weights, it does not give the same thing as the unweighted median of a repeated variable. Both issues are solved by this PR but they are conceptually different.

src/weights.jl

nalimilan · 2019-02-04T13:07:49Z

src/deprecates.jl

+@deprecate wquantile(v::RealVector, w::RealVector, p::RealVector) quantile(v, weights(w), p)
+@deprecate wquantile(v::RealVector, w::RealVector, p::Number) quantile(v, weights(w), [p])[1]
+@deprecate wmedian(v::RealVector, w::AbstractWeights{<:Real}) median(v, w)
+@deprecate wmedian(v::RealVector, w::RealVector) median(v, weights(w))


This makes me realize that we probably shouldn't accept Weights objects in quantile and median since their meaning is ambiguous: they could be frequency weights, or another type of weights. So here we'd better recommend using pweightsor fweights (whichever is closer to the current behavior of wmedian).

Isn't it ok to assume that default weights are probability weights, as it is now? Honestly, most of the time these differences don't matter, and I don't want users starting to feel overwhelmed by the exact weight type they should use.

In Stata:

Each command has its own idea of the "natural" kind of weight. The command will tell you what kind of weight it is assuming and perform the request as if you specified that kind of weight.

Most of the time, but clearly not in this function! :-)

Contrary to Stata, our commands don't "tell" the user what "idea of the 'natural' kind of weight" they have. So it would really not help users to make silent assumptions behind their back. Anyway if people have weights they would better declare them using the right type as early as possible so that they get the right answer automatically.

It's too bad that contrary to Stata our definition of quantiles isn't independent from the type of weights, but at this point we'll just have to bite the bullet.

I'd like that too. I don't think it's too late.

I don't really like commands that talk to the user. Either a command succeeds and it should just do what is requested, or it doesn't and it should indicate a way to make it succeed. Printing warnings during normal operation is just annoying, we'd better explain how to do things properly. Adding f, p or a in front of weights isn't that costly, and anyway if users doesn't know what letter to use they won't understand the warning (and likely do something incorrect).

Also I don't think is common for Julia functions to print messages like that.

I was talking about the second point ;)

src/weights.jl

test/weights.jl

matthieugomez · 2019-02-04T14:32:44Z

Incorporated your suggestion. Sorry I answered to your remarks as new comments — you can see them in "Files Changed".

src/weights.jl

nalimilan · 2019-02-04T17:24:23Z

src/deprecates.jl

+@deprecate wquantile(v::RealVector, w::RealVector, p::RealVector) quantile(v, weights(w), p)
+@deprecate wquantile(v::RealVector, w::RealVector, p::Number) quantile(v, weights(w), [p])[1]
+@deprecate wmedian(v::RealVector, w::AbstractWeights{<:Real}) median(v, w)
+@deprecate wmedian(v::RealVector, w::RealVector) median(v, weights(w))


Most of the time, but clearly not in this function! :-)

Contrary to Stata, our commands don't "tell" the user what "idea of the 'natural' kind of weight" they have. So it would really not help users to make silent assumptions behind their back. Anyway if people have weights they would better declare them using the right type as early as possible so that they get the right answer automatically.

It's too bad that contrary to Stata our definition of quantiles isn't independent from the type of weights, but at this point we'll just have to bite the bullet.

matthieugomez · 2019-02-05T15:25:45Z

Honestly the difference does not matter for 99.99% of vectors. For me it'd be a bit like having to specify the type of quantile (1, 2... 7) etc every time I use the quantile function: while it would be more explicit, it would be much more cumbersome.
In any case, if you still want to do it, maybe it'd be better to do it in a separate pull request?

nalimilan · 2019-02-05T18:18:52Z

Honestly the difference does not matter for 99.99% of vectors. For me it'd be a bit like having to specify the type of quantile (1, 2... 7) etc every time I use the quantile function: while it would be more explicit, it would be much more cumbersome.

That's not really comparable IMHO, as these quantile types all aim to estimate the same quantity (and they converge with large sample sizes). OTC if you have frequency weights and you pass them without noticing they are treated as analytical weights, the interpretation is clearly incorrect. What's the point in spending so much work in finding the right definitions if in the end we don't think it matters? Also, people could be confused if they pass weights(w) for frequency weights and don't get the same result as replicating observations with the unweighted quantile.

In any case, if you still want to do it, maybe it'd be better to do it in a separate pull request?

Adding an error can wait for another PR, but the deprecation should suggest using pweights immediately, so that people don't use something which will throw an error soon.

nalimilan

Thanks!

matthieugomez · 2019-02-18T18:28:20Z

Thanks!

Change the definition of wmedian to wquantile(., 0.5)

c66f39d

matthieugomez added 2 commits December 11, 2018 10:51

deprecate wmedian and wquantile

8b3cb26

Update weights.jl

49cdf8f

put median after quantile

nalimilan mentioned this pull request Feb 1, 2019

Wrong results for weighted quantile #435

Closed

nalimilan reviewed Feb 2, 2019

View reviewed changes

src/weights.jl Outdated Show resolved Hide resolved

src/weights.jl Outdated Show resolved Hide resolved

matthieugomez added 10 commits February 3, 2019 12:57

Update

0b96dd0

Update deprecates.jl

551f440

Update deprecates.jl

2941e1f

Merge branch 'master' into master

a368b4e

rmv i

4ea8934

Merge branch 'master' of https://github.com/matthieugomez/StatsBase.jl

d87239b

Update deprecates.jl

7849064

Update deprecates.jl

6ed920d

Update weights.jl

c709017

add non integer test

6bdcf4e

nalimilan reviewed Feb 4, 2019

View reviewed changes

matthieugomez added 4 commits February 4, 2019 09:08

update

2cb7a5f

pass tests

43e2203

Update weights.jl

fc136db

empty lines

125a30c

nalimilan reviewed Feb 4, 2019

View reviewed changes

matthieugomez added 3 commits February 5, 2019 10:26

Update weights.jl

a7c9086

Update weights.jl

4a20b4c

Update weights.jl

20f04af

matthieugomez added 4 commits February 5, 2019 10:38

Update weights.jl

86b02d0

Update weights.jl

2a53a01

Update weights.jl

8a2ed42

fetch

85dd98a

Use pweights in deprecation

f09e1f8

nalimilan approved these changes Feb 5, 2019

View reviewed changes

nalimilan requested a review from andreasnoack February 5, 2019 18:41

nalimilan merged commit 814219a into JuliaStats:master Feb 18, 2019

nalimilan mentioned this pull request Jan 23, 2022

Throw an error for non-integer FrequencyWeights #756

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the definition of `wmedian` to `wquantile(., 0.5)` #436

Change the definition of `wmedian` to `wquantile(., 0.5)` #436

matthieugomez commented Dec 9, 2018 •

edited

Loading

codecov bot commented Dec 10, 2018

codecov bot commented Dec 10, 2018 •

edited

Loading

nalimilan commented Feb 1, 2019

matthieugomez commented Feb 1, 2019 •

edited

Loading

nalimilan Feb 4, 2019

matthieugomez Feb 4, 2019

nalimilan Feb 4, 2019

matthieugomez Feb 5, 2019

nalimilan Feb 5, 2019

matthieugomez Feb 5, 2019

matthieugomez commented Feb 4, 2019

nalimilan Feb 4, 2019

matthieugomez commented Feb 5, 2019 •

edited

Loading

nalimilan commented Feb 5, 2019

nalimilan left a comment

matthieugomez commented Feb 18, 2019

Change the definition of wmedian to wquantile(., 0.5) #436

Change the definition of wmedian to wquantile(., 0.5) #436

Conversation

matthieugomez commented Dec 9, 2018 • edited Loading

codecov bot commented Dec 10, 2018

Codecov Report

codecov bot commented Dec 10, 2018 • edited Loading

Codecov Report

nalimilan commented Feb 1, 2019

matthieugomez commented Feb 1, 2019 • edited Loading

nalimilan Feb 4, 2019

Choose a reason for hiding this comment

matthieugomez Feb 4, 2019

Choose a reason for hiding this comment

nalimilan Feb 4, 2019

Choose a reason for hiding this comment

matthieugomez Feb 5, 2019

Choose a reason for hiding this comment

nalimilan Feb 5, 2019

Choose a reason for hiding this comment

matthieugomez Feb 5, 2019

Choose a reason for hiding this comment

matthieugomez commented Feb 4, 2019

nalimilan Feb 4, 2019

Choose a reason for hiding this comment

matthieugomez commented Feb 5, 2019 • edited Loading

nalimilan commented Feb 5, 2019

nalimilan left a comment

Choose a reason for hiding this comment

matthieugomez commented Feb 18, 2019

Change the definition of `wmedian` to `wquantile(., 0.5)` #436

Change the definition of `wmedian` to `wquantile(., 0.5)` #436

matthieugomez commented Dec 9, 2018 •

edited

Loading

codecov bot commented Dec 10, 2018 •

edited

Loading

matthieugomez commented Feb 1, 2019 •

edited

Loading

matthieugomez commented Feb 5, 2019 •

edited

Loading