Skip to content

fix accuracy of IteratorRandom::choose() #1059

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 17, 2020
Merged

fix accuracy of IteratorRandom::choose() #1059

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f6eca4
remove unneccessary admonition from choose docs
kazcw Oct 11, 2020
dab98b3
document the ExactSizeIterator optimization in choose()
kazcw Oct 13, 2020
a37a599
accurate IteratorRandom::choose()
kazcw Oct 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/seq/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,15 +296,15 @@ pub trait IteratorRandom: Iterator + Sized {
/// depends on size hints. In particular, `Iterator` combinators that don't
/// change the values yielded but change the size hints may result in
/// `choose` returning different elements.
///
/// For slices, prefer [`SliceRandom::choose`] which guarantees `O(1)`
/// performance.
fn choose<R>(mut self, rng: &mut R) -> Option<Self::Item>
where R: Rng + ?Sized {
let (mut lower, mut upper) = self.size_hint();
let mut consumed = 0;
let mut result = None;

// Handling for this condition outside the loop allows the optimizer to eliminate the loop
// when the Iterator is an ExactSizeIterator. This has a large performance impact on e.g.
// seq_iter_choose_from_1000.
if upper == Some(lower) {
return if lower == 0 {
None
Expand Down Expand Up @@ -336,8 +336,7 @@ pub trait IteratorRandom: Iterator + Sized {
return result;
}
consumed += 1;
let denom = consumed as f64; // accurate to 2^53 elements
if rng.gen_bool(1.0 / denom) {
if gen_index(rng, consumed) == 0 {
result = elem;
}
}
Expand Down Expand Up @@ -963,7 +962,7 @@ mod test {

assert_eq!(choose([].iter().cloned()), None);
assert_eq!(choose(0..100), Some(33));
assert_eq!(choose(UnhintedIterator { iter: 0..100 }), Some(76));
assert_eq!(choose(UnhintedIterator { iter: 0..100 }), Some(40));
assert_eq!(
choose(ChunkHintedIterator {
iter: 0..100,
Expand Down