Skip to content
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

Updating docstrings of distributions #5998

Conversation

vitaliset
Copy link
Contributor

What is this PR about?

In response to issue #5459. Me and @pibieta updated docstrings for some distribution classes inside the multivariate.py module, namely:

  • LKJCholeskyCov
  • LKJCorr
  • MatrixNormal
  • KroneckerNormal
  • CAR

Attention points

  • Inside the KroneckerNormal class, the docstring types of covs, chols and evds were left as list of arrays and list of tuples because the proper type is being developed as @OriolAbril told us.
  • Inside the KroneckerNormal class the type of sigma should be float as far as I understand but we were not sure because the old docstring called it a variable.
  • Inside the CAR class, there was a Numpy matrix input that we changed to ndarray of int. Does this make sense?
  • Inside the MatrixNormal class there were some nxn array that we changed to nxn tensor_like of float. Does that make sense or we should put something else?

Docs / Maintenance

updated docstrings to follow the numpydoc standard

#DataUmbrellaPyMCSprint
cc: @reshamas

@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #5998 (2575662) into main (f9e0dfd) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5998      +/-   ##
==========================================
- Coverage   89.36%   89.29%   -0.07%     
==========================================
  Files          73       72       -1     
  Lines       13253    12884     -369     
==========================================
- Hits        11843    11505     -338     
+ Misses       1410     1379      -31     
Impacted Files Coverage Δ
pymc/distributions/multivariate.py 92.03% <ø> (ø)
pymc/backends/base.py 86.44% <0.00%> (-0.74%) ⬇️
pymc/sampling_jax.py 96.96% <0.00%> (ø)
pymc/step_methods/__init__.py 100.00% <0.00%> (ø)
pymc/step_methods/mlda.py
pymc/distributions/continuous.py 97.89% <0.00%> (+<0.01%) ⬆️
pymc/sampling.py 82.48% <0.00%> (+0.07%) ⬆️
pymc/util.py 77.64% <0.00%> (+0.40%) ⬆️
pymc/model_graph.py 95.45% <0.00%> (+1.75%) ⬆️
pymc/__init__.py 100.00% <0.00%> (+31.57%) ⬆️

…lo as a contributor.

Co-authored-by: pibieta <ibieta.pablo@gmail.com>
@vitaliset
Copy link
Contributor Author

Regarding the W parameter of CAR and the sigma parameter of KroneckerNormal, do you think we should do something @OriolAbril?

@OriolAbril
Copy link
Member

Also, if you look at the preview, the M and N are rendered in pink, which is not good. Can you add them to the numpydoc_xref_ignore set? This will make sure they are shown in bold black font.

@twiecki twiecki merged commit f3ac08b into pymc-devs:main Aug 5, 2022
@twiecki
Copy link
Member

twiecki commented Aug 5, 2022

Thanks @vitaliset!

@vitaliset vitaliset deleted the update_docstring_multivariate_issue5459_data_umbrella branch August 17, 2022 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants