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.

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

Binomial::sample panics with extreme parameters #1378

Open
TheIronBorn opened this issue Jan 31, 2024 · 5 comments
Open

Binomial::sample panics with extreme parameters #1378

TheIronBorn opened this issue Jan 31, 2024 · 5 comments
Labels
T-distributions Topic: distributions

Comments

@TheIronBorn
Copy link
Collaborator

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:

let gen_u = Uniform::new(0., p4).unwrap();

@dhardy dhardy added the T-distributions Topic: distributions label Feb 8, 2024
@dhardy
Copy link
Member

dhardy commented Feb 8, 2024

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

@TheIronBorn
Copy link
Collaborator Author

Working on it now

@TheIronBorn
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@dhardy
Copy link
Member

dhardy commented Mar 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-distributions Topic: distributions
Projects
None yet
Development

No branches or pull requests

2 participants