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

Adding logcdf method to inverse gamma distribution #3944

Merged
merged 7 commits into from
Dec 2, 2020
Merged

Adding logcdf method to inverse gamma distribution #3944

merged 7 commits into from
Dec 2, 2020

Conversation

dfm
Copy link
Contributor

@dfm dfm commented Jun 5, 2020

Supersedes #3873 for paperwork reasons (sorry about the noise!). See more discussion in that thread.

Since this feature requires an update to Theano, it should not be merged until a (possible) future version of PyMC3 that migrates to a pymc-devs-supported version of Theano.

@dfm dfm changed the title adding logcdf method to inverse gamma distribution Adding logcdf method to inverse gamma distribution Jun 5, 2020
@fonnesbeck
Copy link
Member

@dfm we are now running on Theano-PyMC, so is it possible to push this over the finish line?

@dfm
Copy link
Contributor Author

dfm commented Nov 2, 2020

Sure thing! I've fixed the merge conflicts so we'll see if CI passes now. Sorry that I forgot about this one!

@dfm
Copy link
Contributor Author

dfm commented Nov 2, 2020

It looks like there are precision issues when running at float32 so I marked the test as an xfail. If this isn't an acceptable solution, let me know. Otherwise, we should be good to go!

Copy link
Member

@fonnesbeck fonnesbeck left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward. Will merge when tests pass. Thanks!

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #3944 (2cf411e) into master (7ff2f49) will increase coverage by 1.32%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3944      +/-   ##
==========================================
+ Coverage   87.59%   88.91%   +1.32%     
==========================================
  Files          88       89       +1     
  Lines       14312    14433     +121     
==========================================
+ Hits        12536    12833     +297     
+ Misses       1776     1600     -176     
Impacted Files Coverage Δ
pymc3/distributions/continuous.py 93.19% <100.00%> (-0.09%) ⬇️
pymc3/tests/helpers.py 56.60% <0.00%> (-3.75%) ⬇️
pymc3/distributions/multivariate.py 81.20% <0.00%> (-2.13%) ⬇️
pymc3/tuning/starting.py 81.81% <0.00%> (-1.91%) ⬇️
pymc3/distributions/bound.py 91.59% <0.00%> (-0.77%) ⬇️
pymc3/model_graph.py 88.40% <0.00%> (-0.17%) ⬇️
pymc3/distributions/distribution.py 94.42% <0.00%> (-0.15%) ⬇️
pymc3/ode/ode.py 93.75% <0.00%> (-0.07%) ⬇️
pymc3/distributions/dist_math.py 91.79% <0.00%> (-0.04%) ⬇️
... and 23 more

@twiecki
Copy link
Member

twiecki commented Nov 2, 2020

Needs a line in the release-notes.

@canyon289
Copy link
Member

canyon289 commented Nov 22, 2020

@dfm Get a line in the release notes and this will get merged!

@Spaak Spaak added this to the 3.10 milestone Dec 2, 2020
@Spaak Spaak merged commit 988ab9d into pymc-devs:master Dec 2, 2020
@Spaak
Copy link
Member

Spaak commented Dec 2, 2020

Thanks @dfm!

@dfm
Copy link
Contributor Author

dfm commented Dec 2, 2020

whoops - sorry!

Thanks @Spaak, I totally forgot about this!

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.

6 participants