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

Added ICDF for the exponential distribution #6641

Merged
merged 2 commits into from
Apr 1, 2023

Conversation

gokuld
Copy link
Contributor

@gokuld gokuld commented Mar 31, 2023

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

  • ICDF function for the continuous exponential distribution.

Bugfixes

  • ...

Documentation

  • ...

Maintenance

  • Extended test_continuous.py with corresponding tests.

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #6641 (d6a89d5) into main (6404805) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6641   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files          94       94           
  Lines       15923    15927    +4     
=======================================
+ Hits        14643    14647    +4     
  Misses       1280     1280           
Impacted Files Coverage Δ
pymc/distributions/continuous.py 97.70% <100.00%> (+<0.01%) ⬆️

Copy link
Member

@ricardoV94 ricardoV94 left a 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
Copy link
Member

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

Copy link
Contributor Author

@gokuld gokuld Apr 1, 2023

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.

@ricardoV94
Copy link
Member

I suggest you rebase from main instead of merging so that the commits don't show up on the PR (see changed files)

@gokuld
Copy link
Contributor Author

gokuld commented Apr 1, 2023

Thanks, done!

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 1, 2023

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.

@ricardoV94 ricardoV94 merged commit b7764dd into pymc-devs:main Apr 1, 2023
@ricardoV94
Copy link
Member

thanks @gokuld!

@ricardoV94 ricardoV94 changed the title Added ICDF for the continuous exponential distribution. Added ICDF for the exponential distribution Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants