statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
582 stars 83 forks source link

Remove `StatsError` type & replace `Result<T, StatsError>` with `Option<T>` #258

Closed FreezyLemon closed 3 weeks ago

FreezyLemon commented 1 month ago

Mainly looking to move the error discussion forward by seeing how this would look like.

This is a possible solution to #221, and effectively the extreme opposite of #247.

Why do this? Well, a lot of functions look something like this:

fn new(a: f64, b: f64) -> Result<T> {
  if params_valid(a, b) {
    Ok(T { a, b })
  } else {
    Err(StatsError::BadParams)
  }
}

Even if they have more lines, functions often only return one specific error variant, which can be replaced by Option::None. A user can then check the documentation to quickly figure out why it's returning Option::None in most cases (invalid params of some sort).

I opened this as a draft because there are some functions that should probably have a concrete error type, either for future-proofing or because they already return multiple error variants.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 92.33871% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (44ccdb9) to head (71dc1e4). Report is 60 commits behind head on master.

Files with missing lines Patch % Lines
src/statistics/iter_statistics.rs 0.00% 4 Missing :warning:
src/distribution/multinomial.rs 0.00% 3 Missing :warning:
src/function/exponential.rs 57.14% 3 Missing :warning:
src/stats_tests/fisher.rs 78.57% 3 Missing :warning:
src/function/beta.rs 92.00% 2 Missing :warning:
src/function/gamma.rs 94.28% 2 Missing :warning:
src/distribution/dirichlet.rs 83.33% 1 Missing :warning:
src/distribution/multivariate_normal.rs 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #258 +/- ## ========================================== + Coverage 89.49% 89.78% +0.29% ========================================== Files 50 49 -1 Lines 10851 10803 -48 ========================================== - Hits 9711 9700 -11 + Misses 1140 1103 -37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

YeungOnion commented 1 month ago

This is certainly simpler, I do think that construction shouldn't return a Error type with so many variants.

I wish I knew more about the majority use cases for this crate. A None type and pointing to docs is valuable for statisticians/scientists writing analyses in Rust, but if using statrs to write a library, the statically specified errors would be more valuable.

I think all instances you mentioned (only one possible variant from new) would benefit from not returning StatsError in the master branch because it is too broad. Will discuss in #221

FreezyLemon commented 3 weeks ago

I think #265 is more likely at this point, and rebasing this would be a hassle. I'll close this, if it's still on the table I'll re-open or re-do entirely.