Fix weighted sampling without replacement

[bkamins]

A proposal to fix #238. The original article assumes positive weights, so I propose to skip zero weights.
Additionally it is now strictly checked if there are not less positive weights in wv as required sample size.

[ararslan]

It would be great if you could add a test.

[nalimilan]

> 0 should be enough, no?

[nalimilan]

"strictly positive"

[nalimilan]

Same here. Would be more consistent to use > 0 since <= 0 is used below.

[nalimilan]

So now negative weights are silently ignored? Maybe better raise an error for negative weights, which do not have any meaning at the moment? Should also be mentioned in the docs.

[bkamins]

I have added tests and corrected code following @nalimilan suggestions. I did not update docs as I was unsure what would be the best approach to do it (other sample variants do not perform checking for negative values of weights).
Maybe simply stating that in general the behavior of sample with negative weights is unspecified would be best?

[nalimilan]

If other methods don't do the checking, I would just file an issue about this or make a PR so that all methods are consistent. Documenting an unspecified behavior doesn't sound too useful.

[bkamins]

That's why I thought that not documenting it is the best option for now.

If I understand the other methods correctly, in particular sample(wv::WeightVec), they do not check for negative entries, because it would have performance penalty (they use precomputed sum in WeightVec and do not always visit every element in wv).

And the question is do we want to pay this performance penalty (e.g. I understood that the constructor of WeightVec deliberately does not check for non-negativity because of performance reasons and it would be most natural to make this check in the constructor).

[nalimilan]

Thanks for checking. One issue is that negative weights might make sense for some applications but not others, so checking when constructing the weights vector may not be possible. But you're right that for some functions the cost of the check might be significant; could you point me at the method which doesn't go over all individual weights?

I'm not sure what to do. I'd like to hear others' opinions about this.

[bkamins]

As I have mentioned it is in sample(wv::WeightVec). Here is the line:
https://github.com/JuliaStats/StatsBase.jl/blob/master/src/sampling.jl#L354
This function gets called in probably 90% of cases of usage of weighted sampling with replacement (alias method is chosen only for large tasks which are probably rare).

My opinion is that out of the options I have mentioned WeightVec should enforce non-negativity as this is one-time effort (no need to check it later every time WeightVec is used).

One option is to add an argument nonnegative to WeightVec constructor defaulting to true. If it is true then only non-negative weights are allowed. User could explicitly set this argument to false to allow negative weights (and documentation should warn user that most of the methods in StatsBase assume nonnegative weights).

What are uses of negative weights where the sum of weights is relevant? (this is what WeightVec effectively provides now)
The uses of negative weights I know of are in graph contexts but I would not assume that someone would use WeightVec there.

[bkamins]

@nalimilan Some time has passed and there is no feedback. The algorithm of Efraimidis & Spirakis does not support negative weights anyway.

[nalimilan]

Sorry for the delay. Looks good to me apart from one detail I spotted.

[nalimilan]

This isn't needed.

[bkamins]

@nalimilan fixed

[nalimilan]

Thanks!

[nalimilan]

BTW, if you're interested in this part of the code, it seems we forgot to mention them in the list of algorithms provided in the docs at docs/source/sampling.rst.

[nalimilan]

Oops, we had completely forgotten about the need to find a general fix for other methods. I've found a solution which doesn't incur an unnecessary cost when constructing weights vectors if you never use them in a context that needs to check that all entries are positive. See #834.