Skip to content
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

Poisson: Fix undefined behavior and support f64 output #795

Merged
merged 9 commits into from
May 16, 2019
Merged

Poisson: Fix undefined behavior and support f64 output #795

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
31aad14
rand_distr: Check bounds before returning Poisson sample
vks May 15, 2019
c03e2c8
rand_distr: Add support for `f64` to `Poisson`
vks May 15, 2019
638b6be
rand_distr: Make Poisson generic
vks May 15, 2019
eec9bba
rand_distr: Add tests for f64 sampling from Poisson
vks May 15, 2019
90833ca
rand_distr: Add tests for f32 sampling from Poisson
vks May 15, 2019
2553cb5
rand_distr: Encourage inlining of Float methods
vks May 15, 2019
15b9a39
Address review feedback
vks May 15, 2019
84d89a7
Always calculate log_gamma using f64
vks May 15, 2019
ec99801
rand_distr: Inline Poisson sampling
vks May 16, 2019
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
36 changes: 24 additions & 12 deletions rand_distr/src/poisson.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,29 +137,41 @@ mod test {
fn test_poisson_10() {
let poisson = Poisson::new(10.0).unwrap();
let mut rng = crate::test::rng(123);
let mut sum = 0;
let mut sum_u64 = 0;
let mut sum_f64 = 0.;
Copy link
Member

Choose a reason for hiding this comment

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

What is the goal of this test? We know both values should be the same since (a) both should be non-negative integers and (b) we should not be going anywhere close to the limits/accuracy of either type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal is to test that the code path works as expected, not the precision.

Copy link
Member

Choose a reason for hiding this comment

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

It's the same code path, aside from the extra cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to trigger the path with the cast, making sure it works. I think it is important because of the potential undefined behavior.

for _ in 0..1000 {
let s: u64 = poisson.sample(&mut rng);
sum += s;
let s_u64: u64 = poisson.sample(&mut rng);
let s_f64: f64 = poisson.sample(&mut rng);
sum_u64 += s_u64;
sum_f64 += s_f64;
}
let avg_u64 = (sum_u64 as f64) / 1000.0;
let avg_f64 = sum_f64 / 1000.0;
println!("Poisson averages: {} (u64) {} (f64)", avg_u64, avg_f64);
for &avg in &[avg_u64, avg_f64] {
assert!((avg - 10.0).abs() < 0.5); // not 100% certain, but probable enough
}
let avg = (sum as f64) / 1000.0;
println!("Poisson average: {}", avg);
assert!((avg - 10.0).abs() < 0.5); // not 100% certain, but probable enough
}

#[test]
fn test_poisson_15() {
// Take the 'high expected values' path
let poisson = Poisson::new(15.0).unwrap();
let mut rng = crate::test::rng(123);
let mut sum = 0;
let mut sum_u64 = 0;
let mut sum_f64 = 0.;
for _ in 0..1000 {
let s: u64 = poisson.sample(&mut rng);
sum += s;
let s_u64: u64 = poisson.sample(&mut rng);
let s_f64: f64 = poisson.sample(&mut rng);
sum_u64 += s_u64;
sum_f64 += s_f64;
}
let avg_u64 = (sum_u64 as f64) / 1000.0;
let avg_f64 = sum_f64 / 1000.0;
println!("Poisson average: {} (u64) {} (f64)", avg_u64, avg_f64);
for &avg in &[avg_u64, avg_f64] {
assert!((avg - 15.0).abs() < 0.5); // not 100% certain, but probable enough
}
let avg = (sum as f64) / 1000.0;
println!("Poisson average: {}", avg);
assert!((avg - 15.0).abs() < 0.5); // not 100% certain, but probable enough
}

#[test]
Expand Down