Rename sd to sigma for consistency. #3309
Conversation
| _, sd = get_tau_sd(tau=kwargs.pop('tau', None), | ||
| sd=kwargs.pop('sd', None)) | ||
| if 'sd' in kwargs.keys(): | ||
| kwargs['sigma'] = kwargs.pop('sd') |
There was a problem hiding this comment.
should it warn about deprecation?
|
|
||
|
|
||
| def rho2sd(rho): | ||
| def rho2sigma(rho): |
There was a problem hiding this comment.
There was a problem hiding this comment.
Also a backward compat function warning about deprecation is needed
There was a problem hiding this comment.
+1 We should have rho2d = rho2sigma
|
When should we drop support for old convention?
|
|
I'm not sure we should ever drop support, but high uncertainty on what the best course of action is. If we strongly feel that this renaming is the right way forward, we should raise a deprecation warning in the next release and drop it after. |
|
I agree that adopting Greek as a package-wide convention is a good idea. I don't think there's any harm in allowing |
twiecki
force-pushed
the
rename_sd_to_sigma
branch
from
2308a9b to
7cd7cb0
Compare
|
Docs are updated and made a comment in the release notes. |
twiecki
force-pushed
the
rename_sd_to_sigma
branch
2 times, most recently
from
5f4aede to
53e1ce2
Compare
twiecki
force-pushed
the
rename_sd_to_sigma
branch
from
1b74f74 to
cc9f4ef
Compare
A long time ago I made the terrible choice of naming the scale parameter of the Normal distribution
sdinstead ofsigma. This is a terrible choice because all our other parameter names follow greek names, so it should either bemeanandsd, ormuandsigma. Since we're using greek everywhere else it should be the same forsd, I meansigma...This was also rightfully criticized by @bob-carpenter in his blog post https://andrewgelman.com/2017/05/31/compare-stan-pymc3-edward-hello-world/
This so far replaces sd->sigma everywhere in the
continuous.pybut also allows passingsdto keep backwards compatibility. I don't think we should really deprecate this because everyone is using it but allow for a more consistent naming to emerge.If we decide to go ahead we should probably also fix this in the docs, so I marked this as WIP.