statrs-dev / statrs

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

Replace all instances of `StatsError` with custom error types or `Option<T>` #284

Closed FreezyLemon closed 2 months ago

FreezyLemon commented 2 months ago

Closes #221, I think.

Added some new error types similar to the ones introduced in #265.

Some functions only got the simpler Option<T> return type if they only have one error condition. The functions in MultivariateNormal also have a simple Option return type because they're non-public, and it's expected that the preconditions are fulfilled on every call anyways. No need for fancy error messages (and can be easily changed if need be)

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 81.42857% with 26 lines in your changes missing coverage. Please review.

Project coverage is 93.94%. Comparing base (1f8e00a) to head (e87cde0). Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
src/function/beta.rs 84.88% 13 Missing :warning:
src/function/gamma.rs 70.58% 5 Missing :warning:
src/statistics/iter_statistics.rs 0.00% 4 Missing :warning:
src/function/exponential.rs 57.14% 3 Missing :warning:
src/distribution/multivariate_normal.rs 92.85% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #284 +/- ## ========================================== + Coverage 93.75% 93.94% +0.19% ========================================== Files 53 52 -1 Lines 11800 11783 -17 ========================================== + Hits 11063 11070 +7 + Misses 737 713 -24 ```

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

YeungOnion commented 2 months ago

I would drop 9fa380a since we'll go with a panic only in #278 since it's internal.

YeungOnion commented 2 months ago

Wondering if it's valuable to emit a Result<f64, ValueWithConvergence{value: f64, rel_tol: f64}> to describe what convergence was like at the final iteration, or perhaps that would make sense as a part of returning values that emit precision as well as converged value.

FreezyLemon commented 2 months ago

I would drop 9fa380a since we'll go with a panic only in https://github.com/statrs-dev/statrs/pull/278 since it's internal.

I'll leave this PR as-is for now and update it then.. If i revert any of the changes here then I also have to revert the removal of StatsError, which is kind of the end goal of this

Wondering if it's valuable to emit a Result<f64, ValueWithConvergence{value: f64, rel_tol: f64}> to describe what convergence was like at the final iteration

Maybe. I'd say it's a bit out of scope for this PR though, might make sense to open an issue for it

YeungOnion commented 2 months ago

I'll leave this PR as-is for now and update it then.. If i revert any of the changes here then I also have to revert the removal of StatsError, which is kind of the end goal of this

that makes sense, odd that deleting error.rs was a conflict, but I did the rebase locally and merged that last commit removing it.

FreezyLemon commented 2 months ago

@YeungOnion Not sure what happened, but error.rs was not deleted in the last commit. Maybe something went wrong during the rebase?

YeungOnion commented 2 months ago

Presumably, the commit I force pushed added it back into the tree 🙃