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

Feature/corr cholesky [WIP] #27

Merged
merged 3 commits into from
Jul 15, 2022
Merged

Feature/corr cholesky [WIP] #27

merged 3 commits into from
Jul 15, 2022

Conversation

adamhaber
Copy link
Collaborator

This PR implements two different ways to transform a vector of K(K-1)/2 real numbers to a Cholesky factor of a K x K correlation matrix, addressing #24:

  1. Stan's way, based on https://github.com/stan-dev/math/blob/92075708b1d1796eb82e3b284cd11e544433518e/stan/math/rev/fun/cholesky_corr_constrain.hpp#L38-L52

  2. TFP's way, based on https://github.com/tensorflow/probability/blob/v0.17.0/tensorflow_probability/python/bijectors/correlation_cholesky.py

I just want to make sure this is a sensible first step in the proposed workflow; I'll then address the other points raised in #24.

One thing I'm not sure about in the Stan implementation: as far as I understand, in this line we are accounting for the jacobian correction of the tanh transform, whereas in this line we do not; why is that? And, presumably, here we do want to account for that, right? (currently we do not, I need to change that).

Copy link
Collaborator

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Some of these are stylistic comments you can ignore. But it should have a little doc indicating which variable gets a uniform distribution over constrained space and which variable is the unconstrained parameter.

transforms/cholesky/stan_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/stan_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/stan_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/stan_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/stan_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/tfp_transform.stan Outdated Show resolved Hide resolved
transforms/cholesky/tfp_transform.stan Outdated Show resolved Hide resolved
@adamhaber
Copy link
Collaborator Author

Thanks for the review. If this seems alright, I think the reasonable next steps would be:

  1. Compare against TFP and make sure we get the same results (numerically); specifically, make sure we get the log_det_jacobian correction right.
  2. Run initial evaluations.

Sounds good?

@bob-carpenter bob-carpenter merged commit 1f9f4ea into main Jul 15, 2022
@bob-carpenter
Copy link
Collaborator

@admahaber: Yes, that sounds great.

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