Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thow DomainError when sampling weights contain Inf or NaN #678

Closed
wants to merge 1 commit into from
Closed

Thow DomainError when sampling weights contain Inf or NaN #678

wants to merge 1 commit into from

Conversation

xiruizhao
Copy link

Fixes #671

@nalimilan
Copy link
Member

Thanks. Can you add a test?

@xiruizhao
Copy link
Author

I am not sure about this now. Should the check be done in weights.jl? Should I also check for negative weights? Or should the library simply ask the callers to ensure valid inputs?

@nalimilan
Copy link
Member

Good question. In general it's true that non-finite or negative weights are probably not useful. Though it could be the case that in some applications it's OK to allow them, e.g. if observations with NaN weights should be skipped. That's hard to tell. Anyway that would be breaking so we would have first to print a deprecation warning. In the meantime I think it would be good to merge this PR as it fixes a real bug. But feel free to make a more general PR too.

(BTW, I forgot that the same check should be applied to sample! too.)

@xiruizhao xiruizhao changed the title Thow DomainError when sampling weights contain missing, Inf or NaN Thow DomainError when sampling weights contain Inf or NaN Apr 8, 2021
@bkamins
Copy link
Contributor

bkamins commented Jul 27, 2022

Anyway that would be breaking so we would have first to print a deprecation warning.

I would say it is non-breaking if we decide that allowing Inf or NaN is a bug. I would say it is a bug as I cannot imagine any application when allowing it makes sense. The reason is that we guarantee to calculate sum. And sum is Inf or NaN in these cases - which is not well defined. So, by contract of AbstractWeights allowing Inf or NaN should not be allowed I think.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StatsBase.sample should warn about weights containing Inf/NaN
3 participants