Binomial::sample panics with extreme parameters

[TheIronBorn]

let distr = Binomial::new(1 << 31, f64::MIN_POSITIVE).unwrap();
let x = thread_rng().sample(distr);
println!("{}", x);

panics with Uniform::new called with low >= high at: https://github.com/rust-random/rand/blob/1f4507a8e1cf8050e4ceef95eeda8f64645b6719/rand_distr/src/binomial.rs#L180

[dhardy]

A panic in sample is definitely a bug. Would you mind improving the constructor to check for this?

[TheIronBorn]

Working on it now

[TheIronBorn]

I can't find an easy relationship between n and p to make an easy check. Bounds on n * p produce false positives.

So we can:

• use large bounds on n * p and report false positives
• perform the whole BINV setup in new and then test

[TheIronBorn]

I also found some panics in f64_to_i64 so there are multiple dimensions to consider

[dhardy]

Sorry for the delay — there's no obvious best approach here. With that said, I suggest:

• Binomial::new is conservative and uses the bounds on n*p despite false positives, assuming these have a low impact on real use-cases.
• We could possibly add a new constructor like new_unchecked or new_permissive, warning that panic-on-sample is possible.

This is all assuming that it is possible to find reasonable bounds on inputs with a low impact on real usage.

[benjamin-lieser]

One problem is that we enter the BTPE even when np < 10. This happens when n > i32::MAX

I don't know if there are more issues than this, but this could be prevented by rejecting this condition. Maybe we can also fix BINV for n > i32::MAX

BTPE needs this condition, otherwise you get a negative radius in p1