-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Added ICDF for the exponential distribution #6641
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6641 +/- ##
=======================================
Coverage 91.96% 91.96%
=======================================
Files 94 94
Lines 15923 15927 +4
=======================================
+ Hits 14643 14647 +4
Misses 1280 1280
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a comment below about the test
{"lam": Rplus}, | ||
lambda q, lam: st.expon.ppf(q, loc=0, scale=1 / lam), | ||
) | ||
# Custom logp / logcdf / icdf check for invalid parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed, the check*
functions already do this.
The reason we needed special logic for uniform functions was that the helper functions can't automatically figure out what is an invalid parameter, because it depends on both lower and higher.
For these simpler cases the helper functions try domain.lower-1
if you check the source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this makes sense, thanks.
I've deleted the redundant checks and pushed a new commit.
I suggest you rebase from main instead of merging so that the commits don't show up on the PR (see changed files) |
Thanks, done! |
We will squash merge this, but for future reference whenever you rebase it's also a good opportunity to squash together commits that are not important on their own (e.g., fixing a typo introduced in a previous commit or, in this case, reverting the new tests you added in your first commit). If you do rebase -i like in the link, you can replace the pick by squash prefix so that a commit gets squashed with the previous one. |
thanks @gokuld! |
What is this PR about?
Adding the ICDF function for the continuous exponential distribution.
Issue: #6612
Creation of tests for the added ICDF function.
Checklist
Major / Breaking Changes
New features
Bugfixes
Documentation
Maintenance