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

Make StudentT/AsymmetricLaplace .dist() signatures consistent #5628

Merged
merged 22 commits into from
Apr 15, 2022
Merged

Make StudentT/AsymmetricLaplace .dist() signatures consistent #5628

merged 22 commits into from
Apr 15, 2022

Conversation

cluhmann
Copy link
Member

@cluhmann cluhmann commented Mar 20, 2022

This PR closes #5602. Breaking changes:

  • StudentT dist signature changed to match Normal, HalfStudentT, and others
  • StudentT now requires either sigma or lam as kwarg to alert uses to change
  • StudentT's nu argument now defaults to 1.0 (now matches HalfStudentT)
  • HalfStudentT's nu argument now has no default (now matches StudentT)
  • AsymmetricLaplace dist signature changed to match Laplace, SkewNormal, and StudentT
  • AsymmetricLaplace's mu argument no longer has a default value (now matches Laplace)

@cluhmann cluhmann changed the title Distsigs Make StudentT/AsymmetricLaplace .dist() signatures consistent Mar 20, 2022
@codecov
Copy link

codecov bot commented Mar 20, 2022

Codecov Report

Merging #5628 (f33fc00) into main (fa015e3) will increase coverage by 1.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pymc/distributions/continuous.py 98.18% <100.00%> (ø)
pymc/sampling.py 88.15% <0.00%> (+0.11%) ⬆️
pymc/distributions/multivariate.py 92.15% <0.00%> (+0.12%) ⬆️
pymc/model.py 86.02% <0.00%> (+0.41%) ⬆️
pymc/distributions/logprob.py 96.33% <0.00%> (+0.91%) ⬆️
pymc/util.py 76.25% <0.00%> (+1.87%) ⬆️
pymc/distributions/distribution.py 90.80% <0.00%> (+2.29%) ⬆️
pymc/aesaraf.py 91.20% <0.00%> (+2.76%) ⬆️
pymc/backends/ndarray.py 79.64% <0.00%> (+7.07%) ⬆️
pymc/tuning/starting.py 92.68% <0.00%> (+12.19%) ⬆️
... and 6 more

pymc/distributions/continuous.py Outdated Show resolved Hide resolved
pymc/distributions/continuous.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Member

Will need release-notes as well

RELEASE-NOTES.md Outdated Show resolved Hide resolved
@cluhmann
Copy link
Member Author

Should the signatures of get_moment() and logp() match the signature of dist()? I had assumed that the former were used mostly internally, but looking again they seem to be reasonably public. Should have asked earlier.

@ricardoV94
Copy link
Member

Should the signatures of get_moment() and logp() match the signature of dist()? I had assumed that the former were used mostly internally, but looking again they seem to be reasonably public. Should have asked earlier.

No, those should be considered internal, and the order is fixed by the RandomVariable

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 22, 2022

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

RELEASE-NOTES.md Outdated Show resolved Hide resolved
@OriolAbril
Copy link
Member

No, those should be considered internal, and the order is fixed by the RandomVariable

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
Comment on lines 29 to 32
- `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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `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

@@ -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
Copy link
Member

@ricardoV94 ricardoV94 Mar 28, 2022

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

@ricardoV94 ricardoV94 added this to the v4.0.0 milestone Apr 1, 2022
@michaelosthege michaelosthege merged commit 371e944 into pymc-devs:main Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reorder sigma and lam parameter in StudentT distribution
4 participants