-
Notifications
You must be signed in to change notification settings - Fork 430
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.
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
Re: weighted sampling without replacement #1013
Conversation
Hmm, this does not quite work as intended, this test fails: #[test]
fn test_sample_weighted() {
let seed_rng = crate::test::rng;
let v = sample_weighted(&mut seed_rng(423), 10, |i| i as f64, 5).unwrap();
match v {
IndexVec::U32(_) => {},
IndexVec::USize(_) => panic!("expected `IndexVec::U32`"),
}
} |
Should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ach, sorry I left this so long.
I see no action regarding my last comment, but frankly don't think this is necessary just to get this merged. Might be worth making a note somewhere?
An implementation of the A-Res variant should compare well on performance I think (though still O(n)). An implementation of A-ExpJ variant should be the best option for length >> amount. It may be nice having both and selecting which to use with a heuristic, or it may be that A-ExpJ is fast enough to always be a good choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trusting that there are no breaking changes to the algorithm since #976, I'll approve this.
You can look at the commits to see that I kept the original ones and did not modify the algorithm. |
It offers nothing over the "nightly" feature and makes testing more complicated.
Also add some tests.
c327d05
to
d73eac5
Compare
This includes and closes #976.
@dhardy I tried to address your feedback from #976.