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.

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

Refactor Student T Distribution #4694

Merged
merged 8 commits into from
May 14, 2021
Merged

Refactor Student T Distribution #4694

merged 8 commits into from
May 14, 2021

Conversation

themrzmaster
Copy link
Contributor

@themrzmaster themrzmaster commented May 14, 2021 •
edited
Loading

This PR closes subset of #4686.

I am not sure if the parametrization is correct

@twiecki twiecki requested a review from ricardoV94 May 14, 2021 06:28
@twiecki
Copy link
Member

twiecki commented May 14, 2021

Thanks for taking this on @themrzmaster, we'll try to get some eyes on this soon.

@twiecki twiecki mentioned this pull request May 14, 2021
Closed
26 tasks
ricardoV94
ricardoV94 requested changes May 14, 2021
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.

Thanks for opening this PR!
I left some comments. Let me know if you have any questions :)

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved
pymc3/distributions/continuous.py Outdated Show resolved Hide resolved
pymc3/distributions/continuous.py Outdated Show resolved Hide resolved
pymc3/distributions/continuous.py Outdated Show resolved Hide resolved
pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved
@themrzmaster
Copy link
Contributor Author

Thanks @ricardoV94 ! I'll probably get more refactors on the weekend.

@ricardoV94
Copy link
Member

ricardoV94 commented May 14, 2021 •
edited
Loading

Also it seems this test can be re-enabled: https://github.com/pymc-devs/pymc3/blob/1118940dd5238a724be167fcc58295c49e0f10aa/pymc3/tests/test_posteriors.py#L79-L87

This one seems to wrongly mention the StudentT, but I don't think it actually depends on the LKJCholeskyCov, not the StudentT:
https://github.com/pymc-devs/pymc3/blob/1118940dd5238a724be167fcc58295c49e0f10aa/pymc3/tests/test_posteriors.py#L101-L107

@ricardoV94
Copy link
Member

I think that's it. Let's see if the tests pass 🤞

ricardoV94
ricardoV94 previously approved these changes May 14, 2021
View reviewed changes
@ricardoV94
Copy link
Member

ricardoV94 commented May 14, 2021 •
edited
Loading

@themrzmaster I went ahead and changed the default parametrization of the op to be in terms of sigma instead of lam so that we can call scipy directly under the hood.

Let me know if it looks good for you and if there's anything else you want to change!

If you want to make changes, don't forget to first pull from here.

@themrzmaster
Copy link
Contributor Author

LGTM! Thanks

@ricardoV94 ricardoV94 merged commit 3469d23 into pymc-devs:v4 May 14, 2021
@ricardoV94
Copy link
Member

@themrzmaster Great work! Looking forward to your next PR 🚀

twiecki pushed a commit that referenced this pull request Jun 5, 2021
@themrzmaster @ricardoV94 @twiecki
Refactor Student T Distribution (#4694)
fda42f6 
* feat: adapt student t
* Change default parameterization in terms of sigma and tweak tests

Co-authored-by: Ricardo <[email protected]>
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@themrzmaster @twiecki @ricardoV94