Skip to content

Error handling in distribution constructors #581

@dhardy

Description

@dhardy

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    E-questionParticipation: opinions wanted

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions