correct docstrings in Binomial Class

[SangamSwadiK]

What is this PR about?
This PR updates docstrings of Binomial for numpydoc compliance
Issue: #5459

Checklist

• Explain important implementation details 👆
• Make sure that the pre-commit linting/style checks pass.
• Link relevant issues (preferably in nice commit messages)
• Are the changes covered by tests and docstrings?
• Fill out the short summary sections 👇

Major / Breaking Changes

None

Bugfixes / New features

None

Docs / Maintenance

Updates parameters of Binomial Class for numpydoc compliance as per #5459

#DataUmbrellaPyMCSprint2022 @reshamas

[reshamas]

@OriolAbril @cluhmann @twiecki @symeneses

Some general questions I have when reviewing this:

1. Should updates be made at the class level or function level?

2. If it's class, then for line 145, should this be updated too?
   a) value: numeric ==> value : tensor-like of float
   b) description needs a fullstop at the end: Aesara tensor ==> Aesara tensor.

        Parameters
        ----------
        value: numeric
            Value(s) for which log-probability is calculated. If the log probabilities for multiple
            values are desired the values must be provided in a numpy array or Aesara tensor

3. There is no default for value. Is that correct?

4. Interesting that the parameter name is value. Something like x or random_value seems more intuitive, but I supposed that's based from my textbook experience.

[symeneses]

Hi @reshamas,
Regarding to your questions:

1. It should be to the class level including all methods which have already a docstring.
2. Yes, that should be also modified and also in line 169.
3. Correct, there is no default value.

[OriolAbril]

Thanks for the PR @SangamSwadiK! It looks great!

The only change needed to merge the PR is adding the context arg to the plot directive as shown in the 2nd bullet point of the section independent comments.

---

@reshamas and other reviewers

1. As @symeneses said, updates should always be at class and function level, generally including methods (functions defined inside a class) too. I say generally including the methods because for distributions (which is the case here) methods should be ignored. The docstring guide page says:

   You have to review section by section to make sure everything is well documented. If you have chosen a class that is not a distribution (unlike this example where we are working on the pymc.Uniform distribution), you should review the docstrings of all the methods (only if they already exist though, no need to write missing docstrings). Otherwise, you should work only on the function or class docstring. We will therefore ignore the docstrings of the logcdf, get_moment and dist methods.

   The reason for this is that these methods should not be used, the pm.logp or pm.logcdf functions should be used instead and they will call these methods (which might not be methods either in the near future). See for example Refactor old Distribution base class #5308.

2. If it were a not-distribution class, this should also be updated. Here however it is not necessary.

3. Yes, there is not default for value. And here there are no defaults for n or p but there are distributions whose parameters do have default values (defined in .dist method) so in those cases value would continue to have a default argument but the parameters that came after would do even if they are not present in the call signature. This is also related to the strange use (or maybe better non-use) that should be done of those methods and also adds to the reasons for ignoring those methods, at least for now.

4. I wasn't here when that was written so I can't speak to the reasons that were used when defining this originally. I think it might be helpful to note that distributions can be used to define either prior or log likelihood terms so the tensor used as value is not always random from a Bayesian perspective.

[SangamSwadiK]

I've updated with close-figs context.
Thanks !

[reshamas]

Thanks @OriolAbril for explanation on those points.

[codecov]

Codecov Report

Merging #5952 (63810ff) into main (bc8e8cf) will decrease coverage by 2.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5952      +/-   ##
==========================================
- Coverage   89.54%   86.84%   -2.70%     
==========================================
  Files          73       73              
  Lines       13271    13271              
==========================================
- Hits        11883    11525     -358     
- Misses       1388     1746     +358     

Impacted Files	Coverage Δ
pymc/distributions/discrete.py	99.21% <ø> (ø)
pymc/sampling_jax.py	0.00% <0.00%> (-96.96%)	⬇️
pymc/distributions/transforms.py	59.63% <0.00%> (-40.37%)	⬇️
pymc/distributions/dist_math.py	57.71% <0.00%> (-29.72%)	⬇️
pymc/data.py	61.31% <0.00%> (-20.17%)	⬇️
pymc/distributions/logprob.py	93.93% <0.00%> (-3.79%)	⬇️
pymc/aesaraf.py	88.53% <0.00%> (-3.66%)	⬇️
pymc/parallel_sampling.py	85.47% <0.00%> (-0.34%)	⬇️
pymc/model.py	88.07% <0.00%> (-0.14%)	⬇️

[cluhmann]

Sorry for the delay. I was just double-checking the type of n. But everything looks good to me!