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

Add some more inverse CDF functions #260

Merged
merged 7 commits into from
Aug 14, 2024

Conversation

ColmTalbot
Copy link
Contributor

I've been using statrs for a project I'm working on, and noticed that many distributions don't have an explicit implementation of the inverse cdf when they can be simply implemented.

I wasn't sure what the expectations are for documentation and tests, I added some tests for a couple of them, but thought I should just ask if there is a standard I'm missing before writing a bunch of boilerplate.

If this contribution is welcome/desired, I'm happy to put in any docs/tests.

@YeungOnion
Copy link
Contributor

YeungOnion commented Aug 12, 2024

Thanks for the contribution!

For docs we've been writing the mathematical form for expression for inverse cdf's where possible, it more clearly distinguishes them from search implementations.

I think what you've done in the beta distribution for tests is good.

It's been mentioned before to use a "nice" way to test fwd and inverse cdf since a triple of (parameters, x, p) specifies both ways if not specifying precision, but I'd much prefer having these implemented than having a clearer set of tests.


EDIT: specified which distribution the inverse_cdf tests were already written for.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 91.54930% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (44ccdb9) to head (fb182d3).
Report is 7 commits behind head on master.

Files Patch % Lines
src/distribution/chi_squared.rs 0.00% 3 Missing ⚠️
src/distribution/erlang.rs 0.00% 3 Missing ⚠️
src/distribution/beta.rs 96.00% 1 Missing ⚠️
src/distribution/cauchy.rs 96.55% 1 Missing ⚠️
src/distribution/fisher_snedecor.rs 95.45% 1 Missing ⚠️
src/distribution/pareto.rs 93.75% 1 Missing ⚠️
src/distribution/triangular.rs 95.83% 1 Missing ⚠️
src/distribution/weibull.rs 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
+ Coverage   89.49%   89.67%   +0.18%     
==========================================
  Files          50       50              
  Lines       10851    10992     +141     
==========================================
+ Hits         9711     9857     +146     
+ Misses       1140     1135       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ColmTalbot
Copy link
Contributor Author

Thanks, I'll put together the documentation, and some tests that verify the forward and backward match to within reasonable precision.

@ColmTalbot
Copy link
Contributor Author

I wrote some tests and started on docstrings. I'll ping once I finish off the docstrings.

@ColmTalbot ColmTalbot marked this pull request as draft August 13, 2024 15:10
@ColmTalbot ColmTalbot marked this pull request as ready for review August 13, 2024 15:23
@ColmTalbot
Copy link
Contributor Author

@YeungOnion this is now ready for review.

Copy link
Contributor

@YeungOnion YeungOnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me!

Thanks for fixing some of the other docs and catching the precision typo on the test. 10^+14 relative precision isn't any good.

@YeungOnion YeungOnion linked an issue Aug 14, 2024 that may be closed by this pull request
@YeungOnion YeungOnion merged commit 039c545 into statrs-dev:master Aug 14, 2024
8 checks passed
YeungOnion added a commit that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement FisherSnedecor inverse CDF
2 participants