add PosDefCholeskyFactor #38
Conversation
| U[row, col] = x[index] | ||
| index += 1 | ||
| end | ||
| if flag isa NoLogJac |
There was a problem hiding this comment.
You could avoid code duplication here by using transform_with(flag, ...). When flag::NoLogJac, it is "summed" automatically as a no-op.
There was a problem hiding this comment.
transform_with doesn't seem to work for scalar transforms with scalar args:
julia> TransformVariables.transform_with(TransformVariables.NoLogJac(), asℝ₊, 1.0)
ERROR: MethodError: no method matching transform_with(::TransformVariables.NoLogJac, ::TransformVariables.ShiftedExp{true,Float64}, ::Float64)
Closest candidates are:
transform_with(::TransformVariables.NoLogJac, ::TransformVariables.ScalarTransform, ::AbstractArray{T,1} where T<:Real) at /Users/simon/.julia/dev/TransformVariables/src/scalar.jl:15
transform_with(::TransformVariables.LogJac, ::TransformVariables.ScalarTransform, ::AbstractArray{T,1} where T<:Real) at /Users/simon/.julia/dev/TransformVariables/src/scalar.jl:18
transform_with(::TransformVariables.LogJacFlag, ::UnitVector, ::AbstractArray{T,1} where T<:Real) at /Users/simon/.julia/dev/TransformVariables/src/special_arrays.jl:45
...
Stacktrace:
[1] top-level scope at none:0
| @test U <: UpperTriangular | ||
| @test size(U) = (4,4) | ||
| @test inverse(t,U) ≈ v | ||
| end |
There was a problem hiding this comment.
Please
- also test for
Ubeing positive (semi) definite (eg just check the diagonal), - check the log jacobian using ForwardDiff (you find examples of this in the tests),
- test that the results are
@inferred
Happy to help if necessary.
|
Thanks for the PR, I am happy to merge after some minor changes. |
| v = randn(d) | ||
| U = transform(t, v) | ||
| @test U isa UpperTriangular | ||
| @test size(U) = (4,4) |
|
Actually, I've been rethinking this a bit. The problem I'm trying to solve is to get this to play nice with How would you feel about this returning a There is the question then of what the log-Jacobian term should be: i.e. should it be wrt to the uniform density on the cholesky factor itself (as it is now), or should it be wrt to the uniform on the upper triangular part of the "true" matrix (as this is how |
|
Both have their place, just do what you need --- the Stan manual has suggestions for both. But please start a new PR for the different functionality. |
Fixes #6.