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

Difference in event_shape between pyro's LKJCholesky and torch's #3181

Closed
tingiskhan opened this issue Feb 13, 2023 · 2 comments · Fixed by #3199
Closed

Difference in event_shape between pyro's LKJCholesky and torch's #3181

tingiskhan opened this issue Feb 13, 2023 · 2 comments · Fixed by #3199
Labels

Comments

@tingiskhan
Copy link

Issue Description

Event shape of inverse transform of LKJCholesky seems to differ from torch's, and of samples from distribution.

Environment

  • WSL2 Ubuntu 20.04
  • Python 3.9
  • torch 1.13.1
  • pyro 1.8.4+9ed468d

Code Snippet

from torch.distributions import TransformedDistribution, LKJCholesky
from torch.distributions import biject_to

# torch
torch_lkj = LKJCholesky(2, 0.5)
torch_inv = biject_to(torch_lkj.support).inv
torch_transformed = TransformedDistribution(torch_lkj, torch_inv)

# pyro
from pyro.distributions import LKJCholesky as PyroCholesky
from pyro import __version__

print(__version__)

pyro_lkj = PyroCholesky(2, 0.5)
pyro_inv = biject_to(pyro_lkj.support).inv
pyro_transformed = TransformedDistribution(pyro_lkj, pyro_inv)

assert pyro_transformed.event_shape == torch_transformed.event_shape

Stepping through the code, it seems as though pyro.distributions.transforms.cholesky.CorrLCholeskyTransform doesn't implement the inverse_shape method, whereas torch does with torch.distributions.transforms.CorrCholeskyTransform. This results in pyro returning an event shape corresponding to torch.Size([2]), wheras torch returns torch.Size([1]).

@ordabayevy
Copy link
Member

It looks like pyro.distributions.transforms.cholesky.CorrLCholeskyTransform should be deprecated in favor of torch.distributions.transforms.CorrCholeskyTransform. Is that true @fritzo @fehiepsi?

@fehiepsi
Copy link
Member

Yes, I think we can use the upstream one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants