Skip to content

ChiSquared now returns a Gamma random variable #7007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 17, 2023
Merged

ChiSquared now returns a Gamma random variable #7007

merged 6 commits into from
Nov 17, 2023

Conversation

williambdean
Copy link
Contributor

@williambdean williambdean commented Nov 11, 2023 •
edited by ricardoV94
Loading

What is this PR about?
Closes #6994

Checklist

Major / Breaking Changes

  • moment automatically from custom dist

New features

  • N/A

Bugfixes

  • N/A

Documentation

  • N/A

Maintenance

  • ChiSquared used Gamma relationship now

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

@williambdean williambdean changed the title implement helper and remove moment test implement ChiSquared helper Nov 11, 2023
@williambdean williambdean changed the title implement ChiSquared helper implement ChiSquared helper Nov 11, 2023
ricardoV94
ricardoV94 requested changes Nov 12, 2023
View reviewed changes
pymc/distributions/continuous.py Outdated
return _logcdf_helper(Gamma.dist(alpha=nu / 2, beta=0.5), value)
@classmethod
def dist(cls, nu, **kwargs):
return CustomDist.dist(
Copy link
Member

@ricardoV94 ricardoV94 Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need a CustomDist, we can return Gamma directly since it's just a parametrization not a variable transformation.

That mean we need a __new__ method as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with this route as the suggested makes the __repr__ says its just a Gamma distribution. I thought itd be good to display a different name. I can go with your suggestion if you disagree

Copy link
Member

@ricardoV94 ricardoV94 Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't create a new object, indeed it's just a gamma. We can even state so in the docstrings

Copy link
Contributor Author

@williambdean williambdean Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Where in the docs? I'm just seeing the pdf https://www.pymc.io/projects/docs/en/stable/api/distributions/generated/pymc.ChiSquared.html

I will add this relationship in the docstring under the class

ricardoV94
ricardoV94 reviewed Nov 12, 2023
View reviewed changes
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.

Wr should also remove the RV from PyTensor and use only a helper there

@williambdean
Copy link
Contributor Author

Wr should also remove the RV from PyTensor and use only a helper there

sounds good. I will take a look

@codecov Codecov
Copy link

codecov bot commented Nov 12, 2023 •
edited
Loading

Codecov Report

Merging #7007 (facb43b) into main (ec24ce6) will decrease coverage by 0.80%.
Report is 3 commits behind head on main.
The diff coverage is 85.71%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7007      +/-   ##
==========================================
- Coverage   92.19%   91.39%   -0.80%     
==========================================
  Files         101      101              
  Lines       16921    16955      +34     
==========================================
- Hits        15600    15496     -104     
- Misses       1321     1459     +138
Files Coverage Δ
pymc/distributions/continuous.py 97.79% <85.71%> (-0.11%) ⬇️

... and 8 files with indirect coverage changes

@williambdean
Copy link
Contributor Author

New PyMC docs
Screenshot 2023-11-12 at 14 40 19

@williambdean williambdean mentioned this pull request Nov 12, 2023
Merged
6 tasks
@williambdean
Copy link
Contributor Author

I'm interpreting the tests as ones that needs to be removed. For instance, only the check_pymc_draws_match_reference check passes still. One depends on rv_op attribute

The other is for the rewrite which wouldn't be supported anymore through that chisquare

@ricardoV94
Copy link
Member

The other is for the rewrite which wouldn't be supported anymore through that chisquare

Just make it use pm.Chisquare.dist instead of pt.random.chisquare as it was before

@williambdean
Copy link
Contributor Author

All of the pt.random.chisquare are replaced with ChiSquare. Have all tests passing locally

@ricardoV94 ricardoV94 changed the title implement ChiSquared helper ChiSquared now returns a Gamma random variable Nov 17, 2023
@ricardoV94 ricardoV94 added maintenance major Include in major changes release notes section pytensor labels Nov 17, 2023
@ricardoV94 ricardoV94 merged commit 305eb39 into pymc-devs:main Nov 17, 2023
@ricardoV94 ricardoV94 mentioned this pull request Nov 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricardoV94 ricardoV94 ricardoV94 approved these changes

Assignees
No one assigned
Labels
maintenance major Include in major changes release notes section pytensor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove ChiSquared Distribution in favor of helper
2 participants
@williambdean @ricardoV94