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 #3873

Closed
wants to merge 2 commits into from
Closed

Adding logcdf method to inverse gamma distribution #3873

wants to merge 2 commits into from

Conversation

dfm
Copy link
Contributor

@dfm dfm commented Apr 6, 2020

This pull request adds a logcdf method to the inverse gamma distribution. It looks like it'll need to wait on this patch to Theano for the tests to pass, but I wanted to open this anyways and we'll come back to it when it's ready.

An alternative would be to implement this without the gammaincc special function (some combination of gammau and gammaln should do it), but that might have numerical issues.

@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #3873 into master will decrease coverage by 27.66%.
The diff coverage is 20.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3873       +/-   ##
===========================================
- Coverage   90.72%   63.05%   -27.67%     
===========================================
  Files         135      135               
  Lines       21264    21269        +5     
===========================================
- Hits        19292    13412     -5880     
- Misses       1972     7857     +5885     
Impacted Files Coverage Δ
pymc3/tests/test_distributions.py 32.80% <0.00%> (-65.36%) ⬇️
pymc3/distributions/continuous.py 68.91% <25.00%> (-11.19%) ⬇️
pymc3/tests/checks.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_glm.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_ode.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_math.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_memo.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_step.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_util.py 0.00% <0.00%> (-100.00%) ⬇️
pymc3/tests/test_model.py 0.00% <0.00%> (-100.00%) ⬇️
... and 70 more

@dfm dfm changed the title [WIP] Adding logcdf method to inverse gamma distribution Adding logcdf method to inverse gamma distribution Apr 7, 2020
@dfm
Copy link
Contributor Author

dfm commented Apr 7, 2020

The Theano patch has been merged, so this should be good to go once the next version of Theano is released. Is there a procedure for dealing with features like this?

@ColCarroll
Copy link
Member

I was under the impression that theano might not be making new releases any more, and pointing to the github version in case people want bugfixes. @nouiz, is that true?

image

@nouiz
Copy link
Contributor

nouiz commented Apr 7, 2020

I do not plan to make more release and do not know someone that plan to do it. So yes, pointing to the github repo is the right things to do.

@dfm
Copy link
Contributor Author

dfm commented Apr 7, 2020

Got it! I'll let team PyMC3 decide what to do here in that case. This is definitely not a high priority feature. Thanks all!

@rpgoldman
Copy link
Contributor

I thought the plan was to issue a release of pymc-theano, and move to requiring that. @twiecki ?

@twiecki
Copy link
Member

twiecki commented Apr 8, 2020

Yes, we have a fork even though it's not set up properly yet (travis, conda etc). But it would be quite easy for me to put out another theano release from their original one. This might be a good time to switch, however.

@dfm
Copy link
Contributor Author

dfm commented Apr 8, 2020

I'm happy to help with infrastructure for the fork if that would be useful!

@twiecki
Copy link
Member

twiecki commented Apr 8, 2020

@dfm That would be much appreciated! I just added you to the repo, feel free to push ahead without waiting for sign-offs.

https://github.com/pymc-devs/Theano-PyMC

@michaelosthege
Copy link
Member

What's the state of this PR ?
Can we get it done for the 3.9 release?

@dfm
Copy link
Contributor Author

dfm commented May 12, 2020

Since this requires updates to Theano, I'm not sure that it's worth pushing forward on. It depends on how committed the project is to maintaining releases of the Theano fork. I pushed some updates to the fork to get some infrastructure working, but I'm late to the game so I haven't really been following the plans!

I don't actually need this function for the project that I implemented it for after all so it probably shouldn't be a high priority. Feel free to close this if you think that's best!

@michaelosthege
Copy link
Member

michaelosthege commented Jun 5, 2020

In todays lab meeting we discussed that we might switch to the Theano fork with 3.11±, but want to maintain compatibility with the "real" Theano for a while too.

Adding a "don't merge" label and tagging @brandonwillard so we can add it when the compatibility is given.

@dfm
Copy link
Contributor Author

dfm commented Jun 5, 2020

Sounds good! Since I'm an idiot and opened this pull request from my master branch I'm going to close this and open a new pull request from a different branch (looks like you can only change the base branch of an open pull request :/)

@dfm
Copy link
Contributor Author

dfm commented Jun 5, 2020

New PR at #3944. Sorry about the noise!

@dfm dfm closed this Jun 5, 2020
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