Use sigma instead of sd, remove deprecationwarning #4344
Conversation
MarcoGorelli
force-pushed
the
remove-sd
branch
4 times, most recently
from
1c8a4f0
to
24d3a1b
Compare
| if sd is not None: | ||
| sigma = sd | ||
| warnings.warn("sd is deprecated, use sigma instead", DeprecationWarning) | ||
|
|
||
| self.mu = mu = tt.as_tensor_variable(floatX(mu)) | ||
| self.sigma = self.sd = sigma = tt.as_tensor_variable(floatX(sigma)) |
| @@ -832,10 +817,7 @@ class HalfNormal(PositiveContinuous): | |||
| x = pm.HalfNormal('x', tau=1/15) | |||
| """ | |||
|
|
|||
| def __init__(self, sigma=None, tau=None, sd=None, *args, **kwargs): | |||
| if sd is not None: | |||
| sigma = sd | |||
There was a problem hiding this comment.
Instead of deleting, could you raise an error here and advice using sigma
The reason being that sd was a keyword for a REALLY long time so there are a lot of examples out there in the wild with this keyword.
|
Is this the right move? Yes. Does it make me uneasy because I know a ton of people are still happily using |
|
I wonder if we should just keep supporting it in an undocumented fashion and just not tell a soul about it. I certainly can keep a secret. |
|
I support nudging user towards the desired behavior, with the aim to eventually removing |
|
I agree with Thomas: we should just silently allow it to work. I don't think we need error messages, or even warnings, since we are just talking about a parameter naming convention here. |
|
Sure, makes sense! I'll keep the warning in then, and once pymc-devs/pymc-examples#1 is in we can change |
|
I think we can just remove the warning then but still fix it here in all the NBs. |
MarcoGorelli
force-pushed
the
remove-sd
branch
2 times, most recently
from
a08d6b2
to
f2ebb18
Compare
MarcoGorelli
force-pushed
the
remove-sd
branch
from
f2ebb18
to
6169b5b
Compare
MarcoGorelli
changed the title
Codecov Report
@@ Coverage Diff @@
## master #4344 +/- ##
==========================================
+ Coverage 87.95% 88.04% +0.08%
==========================================
Files 88 88
Lines 14501 14482 -19
==========================================
- Hits 12754 12750 -4
+ Misses 1747 1732 -15
|
|
What's up with our tests? |
| tau, sigma = get_tau_sigma(tau=tau, sigma=sigma) | ||
| self.sigma = self.sd = tt.as_tensor_variable(sigma) |
There was a problem hiding this comment.
I think we can leave self.sd too.
There was a problem hiding this comment.
Sure, I can revert this, but wouldn't it be safer to have a single source of truth for the value of sigma?
There was a problem hiding this comment.
I agree with your sentiment, but from user perspective, setting sd=x I would expect there to be a y.sd, and if we're keeping it around, which isn't the cleanest anyway, I don't see what we gain by changing this one thing in a subtle way.
There was a problem hiding this comment.
makes sense, thanks for explaining!
|
Thanks @MarcoGorelli! |
* remove sd deprecation warning * sd -> sigma in pymc3/tests/test_models_linear.py::TestGLM * update cls.sd in tests * noop * don't delete self.sd
No description provided.