Skip to content

Conversation

tvwenger
Copy link
Contributor

@tvwenger tvwenger commented Sep 27, 2023

What is this PR about?
This PR implements a native GammaRV in pymc, which uses the same parameterization as the Gamma distribution (i.e., beta instead of inv_beta). The updated GammaRV in pytensor is included in this dependent PR. This allows the Truncated wrapper to handle this distribution properly and fixes #6931

Also fixes #6938

Checklist

Major / Breaking Changes

Removes dependency on pytensor's GammaRV.

New features

None

Bugfixes

Fixes parameter mismatch between pytensor's GammaRV and pymc's Gamma distribution.

Documentation

Included

Maintenance

N/A


📚 Documentation preview 📚: https://pymc--6934.org.readthedocs.build/en/6934/

@tvwenger tvwenger changed the title add native GammaRV Implement a native GammaRV to support Truncated Gamma distribution Sep 27, 2023
@tvwenger tvwenger changed the title Implement a native GammaRV to support Truncated Gamma distribution Update Gamma Distribution to support pytensor GammaRV reparameterization Sep 27, 2023
@tvwenger
Copy link
Contributor Author

tvwenger commented Sep 27, 2023

I'm updating this PR following the proposed changes to the pytensor PR. While looking at the tests, I stumbled upon

https://github.com/pymc-devs/pymc/blob/e7965fdb8233d564248d925875569f285cc333ce/tests/distributions/test_continuous.py#L2199C1-L2199C45

which I don't think is right!

The logic was right, but the variable names for expected_rv_op_params should have been shape and scale.

@ricardoV94
Copy link
Member

We can add a regression test for the TruncatedGamma bug you uncovered, while we wait for the PyTensor PR to be merged and a new release

@ricardoV94 ricardoV94 changed the title Update Gamma Distribution to support pytensor GammaRV reparameterization Update Gamma Distribution to support new pytensor GammaRV reparameterization Oct 2, 2023
@ricardoV94
Copy link
Member

ricardoV94 commented Oct 2, 2023

@tvwenger there's a commit in #6897 that bumps the PyTensor dependency. We can wait to merge that PR and rebase this one afterwards.

We should still add the regression test with TruncatedGamma

@ricardoV94
Copy link
Member

ricardoV94 commented Oct 2, 2023

Actually it seems we need the Gamma fix first :) I'm going to push stuff here

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #6934 (1f8e470) into main (7c0a095) will decrease coverage by 3.63%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6934      +/-   ##
==========================================
- Coverage   92.15%   88.53%   -3.63%     
==========================================
  Files         100      100              
  Lines       16853    16854       +1     
==========================================
- Hits        15531    14921     -610     
- Misses       1322     1933     +611     
Files Coverage Δ
pymc/data.py 89.47% <100.00%> (ø)
pymc/distributions/continuous.py 97.81% <100.00%> (+<0.01%) ⬆️

... and 23 files with indirect coverage changes

@ricardoV94 ricardoV94 merged commit a3ec9a5 into pymc-devs:main Oct 2, 2023
@welcome
Copy link

welcome bot commented Oct 2, 2023

Congratulations Banner
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Github CI/CD major Include in major changes release notes section pytensor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI error: Mypy errors BUG: Truncated gamma distribution has incorrect logp
2 participants