-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Make StudentT/AsymmetricLaplace .dist() signatures consistent #5628
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5628 +/- ##
==========================================
+ Coverage 86.86% 88.61% +1.74%
==========================================
Files 75 75
Lines 13715 13715
==========================================
+ Hits 11914 12153 +239
+ Misses 1801 1562 -239
|
Will need release-notes as well |
Should the signatures of |
No, those should be considered internal, and the order is fixed by the RandomVariable |
One HalfStudentT test seems to be failing. It probably relied on old parameter order or default, and has to be updated: https://github.com/pymc-devs/pymc/runs/5632254063?check_suite_focus=true |
And these methods should be removed from docs. The only reason they are not is the method listing in classes is automatic. With @ricardoV94 we tried a template to list only (but always) the dist method, but some distributions are actually functions/wrappers that don't have a dist and we weren't able to figure out conditionals in autodoc templates, not sure if we have an issue for that yet though |
RELEASE-NOTES.md
Outdated
- `pm.StudentT` now now requires either `sigma` or `lam` as kwarg [#5628](https://github.com/pymc-devs/pymc/pull/5628) | ||
- `pm.StudentT` now requires `nu` to be specified (no longer defaults to 1) | ||
- `pm.AsymmetricLaplace` positional arguments re-ordered | ||
- `pm.AsymmetricLaplace` now requires `mu` to be specified (no longer defaults to 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `pm.StudentT` now now requires either `sigma` or `lam` as kwarg [#5628](https://github.com/pymc-devs/pymc/pull/5628) | |
- `pm.StudentT` now requires `nu` to be specified (no longer defaults to 1) | |
- `pm.AsymmetricLaplace` positional arguments re-ordered | |
- `pm.AsymmetricLaplace` now requires `mu` to be specified (no longer defaults to 0) | |
Signature and default parameters changed for several distributions (see [#5628](https://github.com/pymc-devs/pymc/pull/5628)): | |
- `pm.StudentT` now requires either `sigma` or `lam` as kwarg | |
- `pm.StudentT` now requires `nu` to be specified (no longer defaults to 1) | |
- `pm.AsymmetricLaplace` positional arguments re-ordered | |
- `pm.AsymmetricLaplace` now requires `mu` to be specified (no longer defaults to 0) |
Also removes a double now
.pre-commit-config.yaml
Outdated
@@ -14,7 +14,7 @@ repos: | |||
exclude: ^requirements-dev\.txt$ | |||
- id: trailing-whitespace | |||
- repo: https://github.com/pre-commit/mirrors-mypy | |||
rev: v0.940 | |||
rev: v0.941 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change shouldn't be in this PR. You might need to rebase from main
This PR closes #5602. Breaking changes:
sigma
orlam
as kwarg to alert uses to changeStudentT'snu
argument now defaults to 1.0 (now matches HalfStudentT)nu
argument now has no default (now matches StudentT)mu
argument no longer has a default value (now matches Laplace)