Throw DomainError when sampling with NaN/Inf weights

[ruairidhs]

[nalimilan]

At https://github.com/JuliaStats/StatsBase.jl/pull/678#issuecomment-814706485 I noted that I wasn't sure whether they can't be legitimate applications where NaN or Inf weights should be allowed, or at least may currently be used without any problems (e.g. if you only work on a subset of the weights vector). This PR is less breaking so it could be good to do it as it fixes a bug. Then we could soft deprecate NaN and Inf weights, without throwing an error right now.

[bkamins]

See what I think in https://github.com/JuliaStats/StatsBase.jl/pull/678#issuecomment-1196928410 (in general I think allowing NaN/Inf is a bug and does not require deprecation period)

[nalimilan]

Fair enough. Nobody complained on Slack so maybe this corner case isn't used in practice.

@ruairidhs Sorry for the change in plans, but would you be willing to update the PR so that it throws an error when passing NaN or infinite values to AbstractWeights constructors and in their setindex! method? Checking that their sum is finite is probably enough.

[bkamins]

Checking that their sum is finite is probably enough.

I agree that this is what we should do (i.e. even if finite values are passed, but they add up to an infinite value I would throw an error)

[ruairidhs]

Fair enough. Nobody complained on Slack so maybe this corner case isn't used in practice.

@ruairidhs Sorry for the change in plans, but would you be willing to update the PR so that it throws an error when passing NaN or infinite values to AbstractWeights constructors and in their setindex! method? Checking that their sum is finite is probably enough.

Yep I can do this

[ruairidhs]

I added a check when constructing AbstractWeights and also for setindex!. Let me know what you think, happy to make changes

[nalimilan]

Thanks!