search

mjhajharia / transforms

2 stars 1 forks source link

Feature/corr cholesky [WIP] #27

Closed adamhaber closed 2 years ago

adamhaber commented 2 years ago

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).

adamhaber commented 2 years ago

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 commented 2 years ago

@admahaber: Yes, that sounds great.