-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Reorder sigma and lam parameter in StudentT distribution #5602
Comments
This seems reasonable to me. Hopefully this will be less of an issue as the docs are updated, but it seems like it should at least be consistent across student, half student, normal, half normal, etc. Are there any other distributions with surprising "defaults" like this? |
I didn't look for more. Do you want to have a look? |
I agree, the reason is that there used to be only the |
I can poke around and see if I see any other weirdness. |
Instead of changing the order, why don't we require these as explicit kwargs? |
That is certainly my practice. Not only are there are alternative ways of specifying a given moment (sigma vs. precision), but there are cases in which there are entire specifications (e.g., gamma) and I don't trust myself to remember which one which is the default one. Ultimately, I think this boils down to a tradeoff between verbosity and explicitness (and I know I value the latter). |
I went through the continuous and discrete distributions. Normal, HalfNormal, TruncatedNormal, and LogNormal all adhere to the same
|
I think those all benefit from being standardized @cluhmann. Would you be interested in opening a PR to fix it? |
Messing with such a central part of the package makes me slightly nervous, but sure. |
It's a bit surprising that lam appears before sigma, when defining a StudentT with positional arguments.
pymc/pymc/distributions/continuous.py
Line 1917 in 09bddde
The docstrings suggest sigma comes before, and this is actually the case for the HalfStudentT. In the Normal, sigma also shows up before tau. I think we should reverse them so that sigma takes precedence here as well.
This would warrant a user warning about the changed behavior, which would need a dummy new variable like
_sigma=None
so that we can detect a positional only argument was used.This emerged recently in discourse: https://discourse.pymc.io/t/pymc3-produces-different-results-than-stan-numpyro/9030/5
The text was updated successfully, but these errors were encountered: