Reject non-finite weights in sample_weighted#1813
Open
teddytennant wants to merge 1 commit into
Open
Conversation
sample_weighted accepted infinite weights but could not sample them correctly: in A-ExpJ an infinite weight gives the key ln(r) / inf == -0.0 for every r, so infinite-weight items cannot be ordered against each other or against finite ones. Once the reservoir's minimum key was -0.0, the skip value became inf and the replacement branch was never taken again, so every item after the first `amount` infinite weights was selected with probability 0 — silently returning the least likely items instead of the most likely. Rather than trying to give infinite weights a meaning, reject them, as NaN and negative weights already are. This changes behavior that was previously asserted in test_multiple_weighted_edge_cases case 6, where a +inf weight was accepted and its item always selected. Supersedes rust-random#1812.
Member
|
Note first that Our current approach breaks down if more than Rejecting any non-finite weight is the simplest solution, but a regression since the current approach is fine with up to |
1 task
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.mdentryRedo of #1812 along the lines you suggested there:
So this drops the separate-reservoir approach entirely and just rejects non-finite weights.
The problem
seq::index::sample_weightedaccepts infinite weights but cannot sample them correctly. In A-ExpJ an infinite weight gives the keyln(r) / inf == -0.0for everyr, so infinite-weight items cannot be ordered against each other, or against finite ones. Once the reservoir's minimum key is-0.0, the skip valuex = ln(r) / -0.0isinfand the replacement branch is never taken again:So the failure mode is a silently wrong result, not an error.
The change
Reject any weight that is not finite and non-negative, which is where NaN and negative weights are already rejected.
-0.0is still accepted (case 8), and sampling among finite weights is untouched.Behavior change
test_multiple_weighted_edge_casescase 6 previously asserted the opposite — that a+infweight is accepted and its item always selected. That assertion is nowWeightError::InvalidWeight. I've also added a case 6b for an infinite weight appearing after the firstamountitems, which is the path that produced the wrong answer above.This is scoped to
seq;WeightedIndexis untouched.cargo test --features std,cargo fmt --checkandcargo clippy --features std,alloc --libare clean.