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

Handle NaN in WeightedIndex with error instead of panic #1005

Merged
merged 1 commit into from Jul 31, 2020

Conversation

wschella
Copy link
Contributor

Implements NaN handling for WeightedIndex weights as discussed in #1004.

Closes #1004

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

I was a bit surprised by the use of the matches! macro. Maybe it is preferable to just use if let here?

src/distributions/weighted_index.rs Outdated Show resolved Hide resolved
@wschella
Copy link
Contributor Author

All requested changes applied and commit amended.

@vks
Copy link
Collaborator

vks commented Jul 30, 2020

It seems my suggested change only works on Rust >= 1.33, sorry about that! We currently support Rust 1.32.

error[E0658]: multiple patterns in `if let` and `while let` are unstable (see issue #48215)
   --> src/distributions/weighted_index.rs:105:9
    |
105 | /         if let Some(Ordering::Less) | None = total_weight.partial_cmp(&zero) {
106 | |             return Err(WeightedError::InvalidWeight);
107 | |         }
    | |_________^

@dhardy Does it make sense to bump our MSRV to 1.33 or should we revert to the old code?

@dhardy
Copy link
Member

dhardy commented Jul 31, 2020

Yes, bumping the MSRV for rand v0.8 makes sense.

@wschella
Copy link
Contributor Author

Sweet. Make sure to let me know if you require anything else from this PR.

@dhardy
Copy link
Member

dhardy commented Jul 31, 2020

I didn't know that was possible either. However, usually we have resolved such cases by inverting the check:

if !(total_weight >= zero) {

Isn't this clearer and easier?

@vks
Copy link
Collaborator

vks commented Jul 31, 2020

Isn't this clearer and easier?

I just did not think of that approach! Certainly seems simpler, and does not require raising the MSRV in this PR to make the tests pass.

@wschella
Copy link
Contributor Author

I agree this preferable, certainly if this conforms to how it's done in other parts of the code base.

I would add a small code comment highlighting that !(x >= y) is not equivalent to x < y for partially ordered types tho, since it's some important implicit logic.

I'll update the PR.

@wschella
Copy link
Contributor Author

Hhhmm it's actually a clippy warning to do it like this, which definitely also makes sense.

https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord

Still change it?

@vks
Copy link
Collaborator

vks commented Jul 31, 2020

We disabled this clippy warning in rand_distr/src/lib.rs, so there is precedence for using the negated comparison. I don't have a strong preference. 🤷

@wschella
Copy link
Contributor Author

I'll just stay consistent then, PR updated.

@dhardy
Copy link
Member

dhardy commented Jul 31, 2020

It's a trick I learned from C++ and found useful here. I agree, one does need to be aware of uncomparable NaNs but it seems like a good option to me. (One also needs to be aware of this without negated comparisons, since as you found NaNs can easily be mishandled.)

@vks
Copy link
Collaborator

vks commented Jul 31, 2020

Looks good, thank you very much!

I agree, one does need to be aware of uncomparable NaNs but it seems like a good option to me. (One also needs to be aware of this without negated comparisons, since as you found NaNs can easily be mishandled.)

Yes, this applies to any PartialOrd type, where a < b == !(a >= b) does not hold in general.

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.

WeightedIndex panics on NaN's
3 participants