rand_distr: Use Result instead of panics

[vks]

This also fixes a few bugs about invalid parameters in the documentation
and the code.

The distributions in rand are untouched. In particular, Uniform and similar still panic.

Refs #581.

[vks]

What is the plan for the distributions in rand that are redundant with rand_distr? Should they be deprecated for 0.7?

[dhardy]

Thanks for getting this started. Do you think #100 has any effect on this? I haven't looked into it recently but some method for supporting lower precision / other types would be nice to have.

[vks]

I think #100 is orthogonal.

[dhardy]

These guards don't catch NaN values, which do sometimes occur in numerical applications. So better I think to replace k <= 0.0 with !(k > 0.0) etc.

[newpavlov]

replace k <= 0.0 with !(k > 0.0) etc.

Floats have is_finite and is_normal methods, so I think it's better to use them.

[dhardy]

That way you need two checks. Infinite values are often acceptable, in which case a single check like !(k > 0.0) suffices.

[vks]

Maybe there should be an error variant for NaN in arguments?

[dhardy]

That's one option, but needs a second round of testing. Is it worthwhile?

[vks]

@dhardy I just changed the checks as you suggested.

[vks]

Note that the error variants here are not yet public, so we could worry about the details later. Or I can still make them public for this PR.

[dhardy]

Interesting point about the error types not being public — why isn't this a compile error (private type in public API)?

Also note that #775 might remove the need for the current error in Normal::new, however we might still want to test against NaN (or not).

[vks]

Interesting point about the error types not being public — why isn't this a compile error (private type in public API)?

The type is public, but its variants are not (similar to a public struct with private members). You can still match on it with _. This is commonly used to make exhaustive matches impossible (by adding a private #[doc(hidden)] __NonExhaustive variant).

Also note that #775 might remove the need for the current error in Normal::new, however we might still want to test against NaN (or not).

This is fine, if we keep the variants private for now we can revisit this later without breakage.

[dhardy]

The type is public

Am I reading something wrong? In rand_distr/src/mod.rs we have lines like pub use self::triangular::Triangular; and mod triangular;, so it doesn't look like triangular::Error gets exported anywhere.

I also don't see anything making the enum variants hidden or private — enums are the exception from the "private by default" rule.

Edit: I suggest making these variants public anyway. Most users won't have a reason to depend on the variants, and tricks like __NonExhaustive are largely an anti-feature in my opinion: if a user wants to match exhaustively, then they want to be told when there is a new item to catch. (Obviously this implies additional variants cannot be added in a patch release.)

[vks]

You are right, the errors are all private. I expected that the modules for the distributions are public, but they are all private.

I suggest making these variants public anyway.

Will do!

[dhardy]

What do you think, are maximally-specific error types like this more useful than a single generic Error with all the variants? More specific enums are useful if matching over the result, however for these constructors I think the only common handler will be to abort on failure, possibly after pretty-printing the message. A single Error type would be significantly less code (esp. after implementing Display) and API.

Another point is that with the export renames, the API doc shows e.g. Pareto::new(..) -> Result<Pareto, Error> and only when you click on Error you see it's named rand_distr::ParetoError (potentially confusing).

Having a single Error type (like std::io::Error) seems a slightly better idea to me, and may be useful anyway for anyone wanting a unified error type.

BTW does this have any overlap with #771? Possibly not since we really don't need this to implement rand_core::Error, though having a unified error type can be useful. In that case we could use a lot of NonZeroU32 codes, but it's probably easier to propagate a cause.

[vks]

What do you think, are maximally-specific error types like this more useful than a single generic Error with all the variants? More specific enums are useful if matching over the result, however for these constructors I think the only common handler will be to abort on failure, possibly after pretty-printing the message. A single Error type would be significantly less code (esp. after implementing Display) and API.

I'm not a fan of mixing all variants in a huge enum, but I think as single struct Error(&'static str) (or struct Error(u32) to save space) would probably work as well. I don't really see a use case for matching on the variants, and it would not be a big deal to reimplement the checks before calling into rand_distr.

On the other hand, I like how the error types currently serve as documentation for the possible failures.

Another point is that with the export renames, the API doc shows e.g. Pareto::new(..) -> Result<Pareto, Error> and only when you click on Error you see it's named rand_distr::ParetoError (potentially confusing).

I could rename the errors or make the modules public and not re-export them.

[dhardy]

I'm not a fan of mixing all variants in a huge enum

Is there some reason or just style? It does avoid the question of whether to add a hidden variant.

struct Error(&'static str)

So long as the internals are private (allowing us to add a code later), this serves our needs sufficiently.

[vks]

Is there some reason or just style? It does avoid the question of whether to add a hidden variant.

It means that the error types are no longer self-documenting, because some variants are unused by a specific distribution, so we have to move the documentation into the distribution's constructor. Also, we have the same problem with the hidden variant, because we still might want to add new variants.

However, it is actually quite similar to Error(&'static str), except that it would be possible to match on the variants.

So long as the internals are private (allowing us to add a code later), this serves our needs sufficiently.

Yes, the internals would be private and it would just be used to print the error messages. Otherwise it would behave like Option as far as the users are concerned.

[dhardy]

Agreed. I don't think there's much reason to match on error variants so lets switch to Error(&'static str) here?

[dhardy]

Lets take this as-is for now, but revisit the error types soon (at least add some From impls).