- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 461
Handle NaN in WeightedIndex with error instead of panic #1005
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
Conversation
There was a problem hiding this 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?
All requested changes applied and commit amended. |
It seems my suggested change only works on Rust >= 1.33, sorry about that! We currently support Rust 1.32.
@dhardy Does it make sense to bump our MSRV to 1.33 or should we revert to the old code? |
Yes, bumping the MSRV for rand v0.8 makes sense. |
Sweet. Make sure to let me know if you require anything else from this PR. |
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? |
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. |
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 I'll update the PR. |
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? |
We disabled this clippy warning in |
I'll just stay consistent then, PR updated. |
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.) |
Looks good, thank you very much!
Yes, this applies to any |
Implements NaN handling for WeightedIndex weights as discussed in #1004.
Closes #1004