Document error cases and edge-case behavior of IndexedRandom sampling methods by teddytennant · Pull Request #1811 · rust-random/rand

teddytennant · 2026-07-11T17:57:50Z

What

Documents the error cases and edge-case behavior of the IndexedRandom sampling methods in src/seq/slice.rs, with doc-test assertions where the behavior is easy to demonstrate:

choose_weighted / choose_weighted_mut / choose_weighted_iter: state that error cases are those of WeightedIndex::new — in particular WeightError::InvalidInput for an empty slice and WeightError::InsufficientNonZero when all weights are zero. The choose_weighted example now asserts both.
sample_weighted: document the WeightError::InvalidWeight case (matching the wording on index::sample_weighted) and that amount is clamped to self.len().
sample: document that amount is clamped to self.len(), with a doc-test asserting sample(&mut rng, 1000).count() == sample.len().
sample_array: document that None is returned if (and only if) N > self.len(), with a doc-test.

Why

Follow-up to #1783. That issue was about choose_weighted claiming to return None for empty sequences when it actually returns Err(WeightError::InvalidInput); the wrong claim was removed, but the docs were left saying nothing about when these methods error. Similarly, the clamping behavior of sample/sample_weighted and the None condition of sample_array were implemented but undocumented, so a reader had to consult the source (or index::sample*) to learn them.

Testing

cargo test --doc seq::slice — all 9 doc-tests pass, including the new/extended ones.
cargo fmt --check and cargo clippy --lib — clean.

Doc-only change; no function bodies are touched.

… methods - choose_weighted, choose_weighted_mut, choose_weighted_iter: document that error cases are those of WeightedIndex::new, with doc-test assertions for the empty-slice and all-zero-weights cases - sample_weighted: document the InvalidWeight error case (matching index::sample_weighted) and that amount is clamped to self.len() - sample: document that amount is clamped to self.len(), with a doc-test - sample_array: document that None is returned iff N > self.len(), with a doc-test Follow-up to rust-random#1783.

Copilot

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

dhardy

Two lines are marginally useful. Most of this is just noise. (To a human who is able to read the docs in the intended docs.rs presentation format, including following links.)

dhardy · 2026-07-13T06:55:13Z

    /// Chooses `amount` elements from the slice at random, without repetition,
    /// and in random order. The returned iterator is appropriate both for
    /// collection into a `Vec` and filling an existing buffer (see example).
+    /// If `amount > self.len()`, `amount` is reduced to `self.len()`.


Suggested change

/// If `amount > self.len()`, `amount` is reduced to `self.len()`.

/// If `amount > self.len()`, all available elements are sampled.

dhardy · 2026-07-13T06:55:47Z

+    ///
+    /// // if the requested amount exceeds the length, every element is sampled:
+    /// assert_eq!(sample.sample(&mut rng, 1000).count(), sample.len());


Suggested change

///

/// // if the requested amount exceeds the length, every element is sampled:

/// assert_eq!(sample.sample(&mut rng, 1000).count(), sample.len());

dhardy · 2026-07-13T06:56:00Z

+    ///
+    /// // None is returned when more elements are requested than are available:
+    /// let b: Option<[u8; 32]> = sample.sample_array(&mut rng);
+    /// assert!(b.is_none());


Suggested change

///

/// // None is returned when more elements are requested than are available:

/// let b: Option<[u8; 32]> = sample.sample_array(&mut rng);

/// assert!(b.is_none());

dhardy · 2026-07-13T06:56:26Z

+    /// Error cases are those of [`WeightedIndex::new`]; in particular, the
+    /// slice being empty results in [`WeightError::InvalidInput`] and all
+    /// weights being zero in [`WeightError::InsufficientNonZero`].
+    ///


This is already documented by the return-type.

Suggested change

/// Error cases are those of [`WeightedIndex::new`]; in particular, the

/// slice being empty results in [`WeightError::InvalidInput`] and all

/// weights being zero in [`WeightError::InsufficientNonZero`].

///

dhardy · 2026-07-13T06:56:36Z

    ///
    /// ```
    /// use rand::prelude::*;
+    /// use rand::seq::WeightError;


Suggested change

/// use rand::seq::WeightError;

dhardy · 2026-07-13T06:57:16Z

+    ///
+    /// [`WeightedIndex::new`]: crate::distr::weighted::WeightedIndex::new


Suggested change

///

/// [`WeightedIndex::new`]: crate::distr::weighted::WeightedIndex::new

dhardy · 2026-07-13T06:58:12Z

    /// elements are never returned; the result may therefore contain fewer
    /// elements than `amount` even when `self.len() >= amount`. The elements
    /// are returned in an arbitrary, unspecified order.
+    /// If `amount > self.len()`, `amount` is reduced to `self.len()`.


This behaviour is documented in sample which is linked here.

Suggested change

/// If `amount > self.len()`, `amount` is reduced to `self.len()`.

dhardy · 2026-07-13T06:58:20Z

+    /// Error cases:
+    /// -   [`WeightError::InvalidWeight`] when a weight is not-a-number or negative.
+    ///


Suggested change

/// Error cases:

/// - [`WeightError::InvalidWeight`] when a weight is not-a-number or negative.

///

dhardy · 2026-07-13T06:58:28Z

+    /// Error cases are those of [`WeightedIndex::new`]; in particular, the
+    /// slice being empty results in [`WeightError::InvalidInput`] and all
+    /// weights being zero in [`WeightError::InsufficientNonZero`].
+    ///


Suggested change

/// Error cases are those of [`WeightedIndex::new`]; in particular, the

/// slice being empty results in [`WeightError::InvalidInput`] and all

/// weights being zero in [`WeightError::InsufficientNonZero`].

///

dhardy · 2026-07-13T06:58:33Z

    /// [`choose_mut`]: IndexedMutRandom::choose_mut
    /// [`choose_weighted`]: IndexedRandom::choose_weighted
    /// [`WeightedIndex`]: crate::distr::weighted::WeightedIndex
+    /// [`WeightedIndex::new`]: crate::distr::weighted::WeightedIndex::new


Suggested change

/// [`WeightedIndex::new`]: crate::distr::weighted::WeightedIndex::new

Copilot AI review requested due to automatic review settings July 11, 2026 17:57

Copilot AI reviewed Jul 11, 2026

dhardy requested changes Jul 13, 2026

View reviewed changes

	/// If `amount > self.len()`, `amount` is reduced to `self.len()`.
	/// If `amount > self.len()`, all available elements are sampled.

	///
	/// // if the requested amount exceeds the length, every element is sampled:
	/// assert_eq!(sample.sample(&mut rng, 1000).count(), sample.len());

		///
		/// [`WeightedIndex::new`]: crate::distr::weighted::WeightedIndex::new

	/// Error cases:
	/// - [`WeightError::InvalidWeight`] when a weight is not-a-number or negative.
	///

Uh oh!

Uh oh!

Conversation

teddytennant commented Jul 11, 2026

What

Why

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

dhardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants