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

Error handling in distribution constructors #581

Closed
dhardy opened this issue Aug 4, 2018 · 2 comments
Closed

Error handling in distribution constructors #581

dhardy opened this issue Aug 4, 2018 · 2 comments
Labels
E-question Participation: opinions wanted
Milestone

Comments

@dhardy
Copy link
Member

dhardy commented Aug 4, 2018

Many distributions currently panic when given invalid parameters:

impl Exp {
    /// Construct a new `Exp` with the given shape parameter
    /// `lambda`. Panics if `lambda <= 0`.
    #[inline]
    pub fn new(lambda: f64) -> Exp {
        assert!(lambda > 0.0, "Exp::new called with `lambda` <= 0");
        Exp { lambda_inverse: 1.0 / lambda }
    }
}

In contrast, quite a few parts of the library return a Result from the ctor. Notably:

impl<X: SampleUniform + PartialOrd> WeightedIndex<X> {
    /// Creates a new a `WeightedIndex` [`Distribution`] ...
    pub fn new<I>(weights: I) -> Result<WeightedIndex<X>, WeightedError> where ... {}
}

/// Error type returned from `WeightedIndex::new`.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum WeightedError {
    /// The provided iterator contained no items.
    NoItem,

    /// A weight lower than zero was used.
    NegativeWeight,

    /// All items in the provided iterator had a weight of zero.
    AllWeightsZero,
}

@vks suggested adding try_new ctors to avoid breakage, but due to #290 proposing to move these to rand_stat there will be some breakage anyway. So which is better?

  1. fn new(...) -> Self and fn try_new(...) -> Result<Self, ...> ctors
  2. fn new(...) -> Self only (status quo) — not ideal for parameters which must be checked at run-time
  3. fn new(...) -> Result<Self, ...> only — requires error handling or .unwrap()

I'm leaning towards the latter despite the breakage. Lots of redundant API functions just for slightly better ergonomics isn't good style and I don't think the breakage is a big deal.

@dhardy dhardy added E-question Participation: opinions wanted T-distributions labels Aug 4, 2018
@vks
Copy link
Collaborator

vks commented Aug 4, 2018

@vks suggested adding try_new ctors to avoid breakage, but due to #290 proposing to move these to rand_stat there will be some breakage anyway.

What breakage? If we reexport rand_stat as rand::distributions there should not be any breakage.

I'm leaning towards the latter despite the breakage. Lots of redundant API functions just for slightly better ergonomics isn't good style and I don't think the breakage is a big deal.

I agree. I think this breakage is much easier to fix (by adding .unwrap()) than many of our previous breakages.

@dhardy dhardy added this to the 0.7 release milestone Aug 23, 2018
@dhardy dhardy mentioned this issue Jan 28, 2019
22 tasks
vks added a commit to vks/rand that referenced this issue Apr 9, 2019
This also fixes a few bugs about invalid parameters in the documentation
and the code.

Refs rust-random#581.
@dhardy
Copy link
Member Author

dhardy commented Jun 5, 2019

This is implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

No branches or pull requests

2 participants