Return Error::Overflow from WeightedIndex on float weight overflow#1808
Open
teddytennant wants to merge 1 commit into
Open
Return Error::Overflow from WeightedIndex on float weight overflow#1808teddytennant wants to merge 1 commit into
teddytennant wants to merge 1 commit into
Conversation
WeightedIndex::new documents Error::Overflow when the sum of weights overflows, but Weight::checked_add_assign never errors for floats, so a sum reaching infinity hit the unwrap on Sampler::new and panicked with NonFinite. The same unwrap in update_weights also panicked after the cumulative weights were already mutated, violating the documented "in case of error, self is not modified" contract. Map the sampler error to Error::Overflow in new, and in update_weights construct the new sampler during the pre-mutation check phase. rust-random#1353 fixed this for integer weights; floats were missed.
dhardy
reviewed
Jul 12, 2026
dhardy
left a comment
Member
There was a problem hiding this comment.
The issue here is that Weight::checked_add_assign was relied on to avoid overflow in fn new, yet it does no such thing for floats. Can you add a note to its docs clarifying that the implementation for floats does not trap overflow-to-inf or nan?
Otherwise, I think this approach is reasonable. Fn update_weights still has a potential issue with integer overflows but lets not address that here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHANGELOG.mdentrySummary
WeightedIndex::newandupdate_weightspanic when the sum of float weights reaches infinity. ReturnError::Overflowinstead, as already documented and as done for integer weights.Motivation
WeightedIndex::newdocumentsError::Overflowwhen the sum of weights overflows, butWeight::checked_add_assignnever errors for floats — the sum just becomes infinite.UniformFloat::newthen rejects the non-finite bound and the.unwrap()onX::Sampler::newpanics:#1353 fixed this for integer weights; float weights were missed.
update_weightshas the same unwrap, and worse: it panics after the cumulative weights have already been mutated, violating the documented "in case of error,selfis not modified" contract.Details
new, map the sampler error toError::Overflow.update_weights, construct the new sampler during the pre-mutation check phase (mapping the error the same way) so a failure leavesselfuntouched, and documentError::Overflowin its error list.overflow_floattest coveringf64::MAX + f64::MAX,f32, an explicitly infinite weight, and that a failedupdate_weightsleaves the distribution unmodified.Tested with
cargo test --lib weighted(14 tests pass);cargo fmt --checkandcargo clippy --libare clean.