Update pymc.TruncatedNormal docstring by purna135 · Pull Request #5546 · pymc-devs/pymc

purna135 · 2022-03-04T18:39:33Z

Related to #5459

This will update the pymc.TruncatedNormal docstring.

codecov · 2022-03-04T18:48:09Z

Codecov Report

Merging #5546 (fbf1190) into main (5eaa516) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5546   +/-   ##
=======================================
  Coverage   87.62%   87.62%           
=======================================
  Files          76       76           
  Lines       13775    13775           
=======================================
  Hits        12070    12070           
  Misses       1705     1705

Impacted Files	Coverage Δ
pymc/distributions/continuous.py	`97.23% <ø> (ø)`

canyon289 · 2022-03-05T02:44:55Z

cc @OriolAbril in case youd like to review

OriolAbril

There are parameters missing. If you look at the dist method you'll see that tau, sd and transform are also accepted. I am not up to date with v4 so I don't know if maybe some of these are deprecated now (which should also be documented via the decorator) their defaults... Can someone weigh in on both deprecations and defaults?

OriolAbril · 2022-03-05T03:40:24Z

pymc/distributions/continuous.py

-    upper: float (optional)
-        Right bound.
+    lower : tensor_like of float, optional
+        Left bound. Defaults to 0.


I think the defaults are wrong (same for upper). I'd bet the default is no bound or infinity or somehting of the sort

I am unable to find out the default value for mu and sigma.
could you please guide me, how to identify these dafult value so that it can help me for other methords.

First check the description in the docstring, sometimes the defaults are incorrectly explained in the extended summary section. If they are not you should read the .dist method and follow the variables defined there and/or function calls to see what the defaults are. Here are some examples, I hope they are illustrative and you can follow the pattern for the rest.

mu

pymc/pymc/distributions/continuous.py

Line 720 in 4223b6e

mu = at.as_tensor_variable(floatX(mu))

it is defined straight away here, so it seems like it is not optional actually. @ricardoV94 is this intended? If the value in the function call were 0 instead of None then you would use the default 0 instead of optional for example, however, if some function is called to get the default, it probably means you should use optional and explain the default (or combinations of defaults depending on other parameters) in the description

lower

pymc/pymc/distributions/continuous.py

Line 724 in 4223b6e

lower = at.as_tensor_variable(floatX(lower)) if lower is not None else at.constant(-np.inf)

the default in the function call is None, and when it is None, what is actually used is - infinity. When a value is given, that value is used. So lower should be lower : tensor_like of float, default -numpy.inf (and check in the readthedocs preview that it gets rendered correctly).

Looks like the default of mu should indeed be zero, not None

transform as a positional argument is outdated an should be removed from the dist signature

pymc/distributions/continuous.py

…catedNormal

purna135 · 2022-03-06T06:37:26Z

Hi @OriolAbril, could you please verify if the doc for sigma and tau is correct.
also I noticed there is no description for sd in pymc.Normal

pymc/pymc/distributions/continuous.py

Line 549 in 4223b6e

def dist(cls, mu=0, sigma=None, tau=None, sd=None, no_assert=False, **kwargs):

and pymc.TruncatedNormal.logp takes mu, sigma, lower, upper as parameter but these are not mentioned in parameter list, should we add these parameter also?

purna135 · 2022-03-06T06:50:08Z

default -numpy.inf is not rendering properly should we use numpy.NINF ?

OriolAbril · 2022-03-08T13:52:16Z

also I noticed there is no description for sd in pymc.Normal

maybe it is deprecated? I don't know why there would be two equivalent arguments 🤔 let's see if someone else knows.

pymc.TruncatedNormal.logp takes mu, sigma, lower, upper as parameter but these are not mentioned in parameter list, should we add these parameter also?

Ignore all the methods (in distributions) that are not .dist. They should not be used directly, there is no need to document them. We are looking into removing them from the docs, but it's a bit tricky.

default -numpy.inf is not rendering properly should we use numpy.NINF ?

Try - numpy.inf, maybe the space fixes the rendering. The uppercase feels a bit strange personally, kind of breaks the symmetry.

OriolAbril · 2022-03-10T18:37:00Z

When both sd and sigma are present, sd should not only not be documented but removed completely, in general. Therefore, here sd should be removed and consequently should not be documented

…catedNormal

purna135 · 2022-03-10T19:19:41Z

When both sd and sigma are present, sd should not only not be documented but removed completely, in general. Therefore, here sd should be removed and consequently should not be documented

Hi @OriolAbril, I reoved sd completly, please have a look if this is the right way to do so.

ricardoV94 · 2022-03-10T19:40:17Z

When both sd and sigma are present, sd should not only not be documented but removed completely, in general. Therefore, here sd should be removed and consequently should not be documented

I think the sd has been debated repeatedly and folks didn't want to remove it last time it was brought up

ricardoV94 · 2022-03-10T20:06:12Z

When both sd and sigma are present, sd should not only not be documented but removed completely, in general. Therefore, here sd should be removed and consequently should not be documented

I think the sd has been debated repeatedly and folks didn't want to remove it last time it was brought up

Seems like people are happy to remove them now. We should probably do it in a separate PR and for all the distributions that have the sigma/sd duplication at once

purna135 · 2022-03-10T20:10:29Z

Seems like people are happy to remove them now. We should probably do it in a separate PR and for all the distributions that have the sigma/sd duplication at once

I am updating all the docstrings, can I remove sd in this PR only ?

purna135 · 2022-03-10T20:12:22Z

https://pymc--5546.org.readthedocs.build/en/5546/api/distributions/generated/pymc.TruncatedNormal.html#pymc.TruncatedNormal

-numpy.inf is still not rendering properly, what to do ?

ricardoV94 · 2022-03-10T20:44:22Z

I am updating all the docstrings, can I remove sd in this PR only ?

I think it's better to leave that for a separate PR

purna135 · 2022-03-10T20:49:31Z

Sure, should I undo the removal of sd in this PR or will I avoid it from the next PR?

ricardoV94 · 2022-03-11T08:46:49Z

Sure, should I undo the removal of sd in this PR or will I avoid it from the next PR?

Yup

OriolAbril · 2022-03-12T06:13:59Z

@purna135 can you undo the sd change here so all sd arguments are removed in #5583? We'll then merge the PR

purna135 · 2022-03-12T06:16:11Z

Sure

purna135 · 2022-03-12T07:03:59Z

Hi @OriolAbril, it's done now, please have a look

…catedNormal

fixed TruncateNormal docsting

df1850c

canyon289 approved these changes Mar 5, 2022

View reviewed changes

OriolAbril requested changes Mar 5, 2022

View reviewed changes

purna135 added 2 commits March 6, 2022 11:06

Merge remote-tracking branch 'upstream/main' into doc_continuous_Trun…

4f6fff4

…catedNormal

fixed TruncateNormal docsting

4a12878

purna135 added 2 commits March 11, 2022 00:48

removed sd from dist

c972983

Merge remote-tracking branch 'upstream/main' into doc_continuous_Trun…

016b88e

…catedNormal

OriolAbril approved these changes Mar 10, 2022

View reviewed changes

ricardoV94 changed the title ~~Update pymc.TruncatedNormal docsting~~ Update pymc.TruncatedNormal docstring Mar 11, 2022

ricardoV94 mentioned this pull request Mar 11, 2022

Remove sd optional kwarg from distributions #5576

Closed

undo the sd change

d36d34c

Merge remote-tracking branch 'upstream/main' into doc_continuous_Trun…

fbf1190

…catedNormal

OriolAbril approved these changes Mar 12, 2022

View reviewed changes

OriolAbril merged commit 7f434bb into pymc-devs:main Mar 12, 2022

purna135 deleted the doc_continuous_TruncatedNormal branch March 12, 2022 10:07

Conversation

purna135 commented Mar 4, 2022

Uh oh!

codecov bot commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

canyon289 commented Mar 5, 2022

Uh oh!

OriolAbril left a comment

Choose a reason for hiding this comment

Uh oh!

OriolAbril Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

purna135 Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

OriolAbril Mar 5, 2022

Choose a reason for hiding this comment

mu

lower

Uh oh!

ricardoV94 Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

purna135 commented Mar 6, 2022

Uh oh!

purna135 commented Mar 6, 2022

Uh oh!

OriolAbril commented Mar 8, 2022

Uh oh!

OriolAbril commented Mar 10, 2022

Uh oh!

purna135 commented Mar 10, 2022

Uh oh!

ricardoV94 commented Mar 10, 2022

Uh oh!

ricardoV94 commented Mar 10, 2022

Uh oh!

purna135 commented Mar 10, 2022

Uh oh!

purna135 commented Mar 10, 2022

Uh oh!

ricardoV94 commented Mar 10, 2022

Uh oh!

purna135 commented Mar 10, 2022

Uh oh!

ricardoV94 commented Mar 11, 2022

Uh oh!

OriolAbril commented Mar 12, 2022

Uh oh!

purna135 commented Mar 12, 2022

Uh oh!

purna135 commented Mar 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 4, 2022 •

edited

Loading