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

Implement a CorrCholesky transformation for LKJCorr #13

Closed
junpenglao opened this issue May 9, 2019 · 6 comments
Closed

Implement a CorrCholesky transformation for LKJCorr #13

junpenglao opened this issue May 9, 2019 · 6 comments
Labels
enhancements New feature or request feature request help wanted Extra attention is needed

Comments

@junpenglao
Copy link
Member

Came across this issue in TFP: tensorflow/probability#400 and found out that our transformation of LKJ is just a interval transformation - that's incorrect as it will produce invalid correlation matrix right?
https://github.com/pymc-devs/pymc3/blob/master/pymc3/distributions/multivariate.py#L1158-L1159

@aseyboldt
Copy link
Member

Our LKJCorr will return a logp of -inf in some cases, yes. The proper transformation was hard to implement in theano, that's why I wrote LKJCholescyCov.
Unless we implement it however, we should probably deprecate pm.LKJCorr.

@junpenglao
Copy link
Member Author

That makes sense, should we revisit the transformation for correlation matrix now since there is a version in TFP? It would need some theano.scan magic but it should be doable.

@junpenglao
Copy link
Member Author

@junpenglao junpenglao changed the title LKJ transformation Implement a CorrCholesky transformation for LKJCorr Jun 3, 2020
@junpenglao
Copy link
Member Author

Proposal:

  • implement a LKJCorrCholesky (or add a Cholesky parameterization to LKJCorr and make that the default)
  • implement a CorrCholesky transformation
  • clean up the deprecation/warning related to LKJCorr

Regarding the transformation, we can opt to use the simplier version in TFP. Some related discussion see:
tensorflow/probability#694 (comment)

@twiecki
Copy link
Member

twiecki commented Jul 20, 2021

Bumping this issue, looks like a transform is pretty simple: https://github.com/pyro-ppl/numpyro/blob/master/numpyro/distributions/transforms.py#L432 and we have a LKJCov now.

@ricardoV94
Copy link
Member

Closing in favor of pymc-devs/pymc#7101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancements New feature or request feature request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants