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

Add gufunc signature to RandomVariable's docstrings #1160

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

rlouf
Copy link
Member

@rlouf rlouf commented Aug 31, 2022

In this PR I add gufunc signatures to the RandomVariables docstrings. The notation is clear and concise; with knoweldge of broadcasting rules and the definition of size the signature makes shapes predictible.

Most distributions behave like Elemwise Ops, with the exception of:

  • Multinomial (), (n) -> (n)
  • Dirichlet (k) -> (k)
  • MvNormal (n), (n,n) -> (n)
  • Categorical (p) -> ()
  • Choice (x) -> ()
  • Permutation (x) -> (x)

@rlouf rlouf changed the title Add gufunc signature to RandomVariable\'s docstrings Add gufunc signature to RandomVariable's docstrings Aug 31, 2022
@rlouf rlouf added documentation Improvements or additions to documentation random variables Involves random variables and/or sampling labels Aug 31, 2022
@brandonwillard
Copy link
Member

This work is part of #695. We need to create a single, consistent interface for this information and apply it to all the (effectively) block-wise Ops—which includes all the scalar Ops used by Elemwise (i.e. we need to replace all the nfunc_specs).

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #1160 (ac3e073) into main (6232637) will increase coverage by 0.21%.
The diff coverage is 100.00%.

❗ Current head ac3e073 differs from pull request most recent head 4b3af51. Consider uploading reports for the commit 4b3af51 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1160      +/-   ##
==========================================
+ Coverage   79.14%   79.35%   +0.21%     
==========================================
  Files         173      161      -12     
  Lines       48528    48282     -246     
  Branches    10322    10962     +640     
==========================================
- Hits        38408    38316      -92     
+ Misses       7628     7454     -174     
- Partials     2492     2512      +20     
Impacted Files Coverage Δ
aesara/tensor/random/basic.py 99.00% <100.00%> (ø)
aesara/sparse/type.py 72.89% <0.00%> (-8.69%) ⬇️
aesara/link/vm.py 90.26% <0.00%> (-2.30%) ⬇️
aesara/link/numba/dispatch/scalar.py 86.00% <0.00%> (-1.34%) ⬇️
aesara/compile/function/pfunc.py 81.40% <0.00%> (-1.01%) ⬇️
aesara/link/utils.py 61.12% <0.00%> (-0.72%) ⬇️
aesara/printing.py 49.76% <0.00%> (-0.41%) ⬇️
aesara/ifelse.py 51.00% <0.00%> (-0.29%) ⬇️
aesara/tensor/var.py 88.06% <0.00%> (-0.19%) ⬇️
aesara/tensor/basic.py 90.04% <0.00%> (-0.10%) ⬇️
... and 33 more

@rlouf
Copy link
Member Author

rlouf commented Sep 1, 2022

Regardless of how we implement the interface we need to communicate how the current shape systems works to the users in the documentation, and I think gufunc signatures are the right tool for that.

Copy link
Contributor

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those two were wrong

aesara/tensor/random/basic.py Outdated Show resolved Hide resolved
aesara/tensor/random/basic.py Outdated Show resolved Hide resolved
@rlouf
Copy link
Member Author

rlouf commented Sep 13, 2022

They were!

ricardoV94
ricardoV94 previously approved these changes Sep 13, 2022
@rlouf
Copy link
Member Author

rlouf commented Sep 13, 2022

Squashed the commits.

@rlouf rlouf force-pushed the rv-gufunc-signature branch 2 times, most recently from 8fd15a0 to 63410a7 Compare September 26, 2022 14:54
@rlouf
Copy link
Member Author

rlouf commented Sep 28, 2022

Should we merge this? This documents the current behavior of RandomVariables and as such is useful even before #695 is merged.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how many of these are scalars and, as a result, have trivial signatures, it's hard to say how useful those particular cases are.

Regardless, it doesn't hurt to add them, but we need to do something about the gufunc/RandomVariable comment before that.

doc/library/tensor/random/basic.rst Outdated Show resolved Hide resolved
brandonwillard
brandonwillard previously approved these changes Oct 4, 2022
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
@brandonwillard brandonwillard merged commit a619a4e into aesara-devs:main Oct 5, 2022
@rlouf rlouf deleted the rv-gufunc-signature branch October 5, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation random variables Involves random variables and/or sampling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants