Skip to content
Snippets Groups Projects
Commit e5311d6c authored by STEVAN Antoine's avatar STEVAN Antoine :crab:
Browse files

fix random item draw (dragoon/komodo!127)

related to
- !122

## description
!122 introduced a `draw_unique_indices` function which uses a `HashSet` to accumulate unique indices in the range `0..<len`.
however, a `HashSet` does not preserve the order of insertion when iterating over the elements of the set... which results in apparent randomness, even though the RNG seed is the same :open_mouth: 

this MR switches back to using `shuffle` which used to work, even though a bit less performant :ok_hand:   
it's basically a revert of !122, while keeping the refactoring into `random.rs`.

## measuring the performance
i did run the same timing experiment from !122 but with `main` on `bb55005f` and the MR on `fix-shuffle`

| env     | main                   | mr                      | improvement         |
| ------- | ---------------------- | ----------------------- | ------------------- |
| fixed:0 | 6sec 244ms 238µs 45ns | 8sec 734ms 929µs 328ns | -39.88783363238997  |
| fixed:1 | 639ms 720µs 39ns      | 731ms 360µs 261ns      | -14.325051024390373 |

we loose a bit
parent bb55005f
No related branches found
No related tags found
No related merge requests found
use rand::{Rng, RngCore}; use rand::{seq::SliceRandom, Rng, RngCore};
use std::collections::HashSet; use std::collections::HashSet;
#[allow(dead_code)]
fn draw_unique_indices(n: usize, vec_len: usize, rng: &mut impl RngCore) -> HashSet<usize> { fn draw_unique_indices(n: usize, vec_len: usize, rng: &mut impl RngCore) -> HashSet<usize> {
let mut indices = HashSet::new(); let mut indices = HashSet::new();
...@@ -17,9 +18,8 @@ pub(super) fn draw_unique_elements<T: Clone>( ...@@ -17,9 +18,8 @@ pub(super) fn draw_unique_elements<T: Clone>(
n: usize, n: usize,
rng: &mut impl RngCore, rng: &mut impl RngCore,
) -> Vec<T> { ) -> Vec<T> {
let mut res = vec![]; let mut things = things.to_vec();
for i in draw_unique_indices(n, things.len(), rng) { things.shuffle(rng);
res.push(things[i].clone());
} things.iter().take(n).cloned().collect()
res
} }
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment