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

Batched MvNormal distribution #5424

Closed
wants to merge 2 commits into from

Conversation

Sayam753
Copy link
Member

@Sayam753 Sayam753 commented Jan 30, 2022

Thank your for opening a PR!

Initializing the work on #5383

Blocked by aesara-devs/aesara#798
EDIT: Now, I am correctly using at.broadcast_shape function.

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes?
    This PR generalizes the MvNormal distribution.

The parameters mu and cov were restricted to 2d. To remove this restriction, I created a custom Op that operates on batches of square matrices to compute their inverses in vectorized fashion.

With the changes in this PR, I think that random variable object for MvNormal distribution can be created and random samples can be drawn. The logp computations still need to be sorted out. So, this PR is WIP.

@Sayam753 Sayam753 marked this pull request as draft January 30, 2022 15:53
Comment on lines +9 to +14
class BatchedMatrixInverse(Op):
"""Computes the inverse of a matrix.

`aesara.tensor.nlinalg.matrix_inverse` can only inverse square matrices.
This Op can inverse batches of square matrices.
"""
Copy link
Member Author

Choose a reason for hiding this comment

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

Two questions:

  • Is there an existing function in aesara that can inverse batches of square matrices in a vectorized way?
  • If no, I have added this Op in a new file. Where should this class ideally be placed?

Copy link
Member

Choose a reason for hiding this comment

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

distributions/dist_math.py?

Copy link
Member

@ricardoV94 ricardoV94 Jan 30, 2022

Choose a reason for hiding this comment

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

If numpy has support for batched matrix inversion but not Aesara, we can open an issue there. Here is a similar case: aesara-devs/aesara#791

Copy link
Member

Choose a reason for hiding this comment

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

Actually the Aesara Op is just doing the same as this one but has an unnecessary ndim check?

We should just open a PR to fix it there

@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #5424 (5ee641f) into main (73add2d) will decrease coverage by 6.64%.
The diff coverage is 68.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5424      +/-   ##
==========================================
- Coverage   81.38%   74.74%   -6.65%     
==========================================
  Files          82       83       +1     
  Lines       14207    14241      +34     
==========================================
- Hits        11563    10645     -918     
- Misses       2644     3596     +952     
Impacted Files Coverage Δ
pymc/distributions/multivariate_utils.py 61.76% <61.76%> (ø)
pymc/distributions/multivariate.py 71.76% <90.90%> (-19.77%) ⬇️
pymc/backends/tracetab.py 28.57% <0.00%> (-71.43%) ⬇️
pymc/func_utils.py 22.50% <0.00%> (-65.00%) ⬇️
pymc/printing.py 22.22% <0.00%> (-63.64%) ⬇️
pymc/distributions/censored.py 37.28% <0.00%> (-55.94%) ⬇️
pymc/distributions/bound.py 44.32% <0.00%> (-55.68%) ⬇️
pymc/exceptions.py 58.62% <0.00%> (-41.38%) ⬇️
pymc/distributions/discrete.py 68.64% <0.00%> (-31.12%) ⬇️
pymc/distributions/continuous.py 75.88% <0.00%> (-21.05%) ⬇️
... and 13 more

@Sayam753 Sayam753 marked this pull request as ready for review January 30, 2022 16:07
@Sayam753 Sayam753 marked this pull request as draft January 30, 2022 16:08
@ricardoV94
Copy link
Member

ricardoV94 commented Aug 29, 2022

@Sayam753 @purna135 Shall we close this one while the required changes in Aesara are not implemented?

@Sayam753
Copy link
Member Author

Thanks for the ping Ricardo.
Yes. This can be closed now and revived when we have Blockwise implemented in Aesara.

@Sayam753 Sayam753 closed this Aug 29, 2022
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.

2 participants