Skip to content

Fix sample_weighted excluding items after the first amount infinite weights#1812

Closed
teddytennant wants to merge 1 commit into
rust-random:masterfrom
teddytennant:weighted-sample-infinite-weights
Closed

Fix sample_weighted excluding items after the first amount infinite weights#1812
teddytennant wants to merge 1 commit into
rust-random:masterfrom
teddytennant:weighted-sample-infinite-weights

Conversation

@teddytennant

Copy link
Copy Markdown
Contributor
  • Added a CHANGELOG.md entry

Summary

seq::index::sample_weighted (and therefore sample_weighted on slices) never samples any item that appears after the first amount items with infinite weight. Those first amount infinite-weight items win deterministically; every later item — finite or infinite weight — is selected with probability 0.

Motivation

In sample_efraimidis_spirakis, an infinite weight produces the key ln(r) / inf = -0.0. Once the reservoir's minimum key is -0.0, the skip value x = ln(r) / -0.0 = inf, so x -= weight leaves x at inf for finite weights and NaN for infinite ones, and the replacement branch is never taken again:

let mut rng = rand::rng();
let weights = [1.0, 1.0, f64::INFINITY, f64::INFINITY, f64::INFINITY];
let v = rand::seq::index::sample_weighted(&mut rng, 5, |i| weights[i], 2).unwrap();
// always [0, 1]: the two finite-weight items, each of relative weight 0

Infinity is otherwise accepted as a weight (only w < 0 and NaN are rejected), so this silently returns the least likely items instead of the most likely ones.

Details

Infinite-weight items cannot be represented by A-ExpJ keys (any such key is -0.0), so they are diverted into a separate uniform reservoir (Algorithm R) that takes priority over the heap of finite-weight candidates. The resulting semantics, now documented on both sample_weighted variants: items with infinite weight have priority over items with finite weight and are sampled uniformly among themselves. Sampling among finite weights is unchanged, and the case of at most amount infinite weights appearing early keeps its old behavior (those items were and are always selected).

Tests:

  • seq::index::test_sample_weighted_infinite: all-infinite weights are sampled uniformly (count check over 1000 runs); infinite weights dominate finite ones; and the failing case above — infinite weights appearing after amount finite ones — now excludes the finite items and picks uniformly among the infinite ones.
  • seq::slice::test_multiple_weighted_edge_cases: added a case with more infinite weights than amount through the slice API.

cargo test --features std, cargo fmt --check and cargo clippy --features std,alloc --lib are clean.

… weights

In sample_efraimidis_spirakis, an infinite weight produces the key
ln(r) / inf = -0.0. Once the reservoir's minimum key is -0.0, the
skip value x = ln(r) / -0.0 = inf, so x -= weight leaves x at inf for
finite weights and NaN for infinite ones, and the replacement branch
is never taken again: the first `amount` infinite-weight items win
deterministically and every later item is sampled with probability 0.

Divert infinite-weight items into a separate uniform reservoir
(Algorithm R) that takes priority over the A-ExpJ heap of finite
weights. Sampling among finite weights is unchanged.
Copilot AI review requested due to automatic review settings July 11, 2026 18:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dhardy

dhardy commented Jul 13, 2026

Copy link
Copy Markdown
Member

In the case that some input weights overflow to inf (while others don't) no approach will yield the expected results. If anything, non-finite weights should be considered an error; we certainly don't want this approach.

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.

3 participants