API differences between Uniform and Bernoulli

[Danvil]

Hello,

distributions::Uniform::from panics when the given range is invalid.

In contrast distributions::Bernoulli::new returns an Option object if the given probability is invalid.

It comes to a surprise that seemingly similar functions have different API design. Is there a particular reason for this?

[dhardy]

Well, Uniform::new and new_inclusive should probably return a Result — that's something to change for 0.9.

Rng::gen_range should continue to panic on invalid ranges, which fits with other Rng methods better.

But Uniform::from? Implementing From<Range<X>> for Option<Uniform<X>> isn't ergonomic to use, and really from is just a convenience wrapper around new / new_inclusive, so no, these should continue to panic on error.

[Danvil]

Would that mean Bernoulli::new or Bernoulli::from_ratio should panic if the probability is invalid?

[dhardy]

No; those are construction methods on the type. Uniform::from isn't, but rather an implementation of From...

Returning a Result over panic is a more recent change (see Bernoilli v0.6); there is some possibility of wider usage of Result in the API, but on Uniform::from this is unlikely (we could add TryFrom support in addition however).

This needs further thought/discussion before resolution.

[Danvil]

Makes sense. At the moment I see:
Uniform::new: panics -- creates from interval passed as two numbers
Uniform::from: panics -- "converts" from a Range object
Bernoulli::new: Result -- creates from one number
Bernoulli::from_ratio: Result -- creates from two numbers representing a rational number

Three suggested changes:

• Decide if panic or Result is used for error cases. Then either change Uniform::new to return Result or Bernoulli::new to panic to make the API coherent.
• Remove Bernoulli::from_ratio as it converts to a single float internally anyway (((f64::from(numerator) / f64::from(denominator)) and just clutters the API.
• Add Bernoulli::from for num::rational::Ratio to provide a replacement for from_ratio. This function would panic as the From trait requires.

[dhardy]

• We'd prefer ordinary constructors to return a Result: see Bernoulli constructor #799. Somehow we forgot Uniform.
• I disagree about Bernoulli::from_ratio being just clutter. See uses.
• num is not an existing dependency of rand, and it would take quite a significant argument to change that (we've had several complaints about too many dependencies). Also, Bernoulli::from(num::rational::Ratio::new(a, b)) is a bit more clunky to use.

[vks]

Somehow we forgot Uniform.

Not really, we did not change the constructors of distributions in rand, because we wanted to avoid breaking changes in rand at the time. In rand_distr, breaking changes are less likely to cause churn, so we only changed the distributions there.

However, I agree we should fix this for Rand 0.9.