From 9dd33cd86b320f9b17caf78c8b2f7037936f2518 Mon Sep 17 00:00:00 2001 From: Hong Ge Date: Sat, 22 Aug 2026 22:27:14 +0100 Subject: [PATCH 1/6] Fix log-Jacobian for dynamic-to-fixed transforms --- src/transformed_values.jl | 5 +---- test/transformed_values.jl | 4 ++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/transformed_values.jl b/src/transformed_values.jl index 61f2f31de..36afed56b 100644 --- a/src/transformed_values.jl +++ b/src/transformed_values.jl @@ -348,10 +348,7 @@ function apply_transform_strategy( fwd_transform = inverse(target.transform) transformed_value, logjac = with_logabsdet_jacobian(fwd_transform, raw_value) transformed_tv = TransformedValue(transformed_value, target) - # TODO: https://github.com/TuringLang/DynamicPPL.jl/issues/1407 - # Likely should return `logjac` rather than `logjac - inv_logjac`; the sibling - # branches all return only the target's forward Jacobian. - (raw_value, transformed_tv, logjac - inv_logjac) + (raw_value, transformed_tv, logjac) else error("unknown target transform: $target") end diff --git a/test/transformed_values.jl b/test/transformed_values.jl index e79ab3ee4..8e5808e29 100644 --- a/test/transformed_values.jl +++ b/test/transformed_values.jl @@ -241,6 +241,10 @@ end # Matching transform should work strategy_ok = DynamicPPL.WithTransforms(VarNamedTuple(; x=ft), UnlinkAll()) + _, _, dynamic_to_fixed_logjac = DynamicPPL.apply_transform_strategy( + strategy_ok, TransformedValue(linked_val, DynamicLink()), vn, dist + ) + @test dynamic_to_fixed_logjac ≈ logjac new_raw, new_tv, new_logjac = DynamicPPL.apply_transform_strategy( strategy_ok, tv, vn, dist ) From 1271b59641c9bfd136997d2c8aa9dffa83b69671 Mon Sep 17 00:00:00 2001 From: Hong Ge Date: Sat, 22 Aug 2026 22:33:40 +0100 Subject: [PATCH 2/6] Bump patch version to 0.42.5 --- HISTORY.md | 4 ++++ Project.toml | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/HISTORY.md b/HISTORY.md index d290d5996..5d4cf6d5e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,7 @@ +# 0.42.5 + +Fixed `apply_transform_strategy` to accumulate only the target fixed transform's forward log-Jacobian when converting from `DynamicLink`. Previously, the source inverse Jacobian was also included, producing incorrect internal log densities. + # 0.42.4 `arraydist` on a vector of univariate distributions now builds its `Distributions.Product` through the inner constructor instead of `Product(dists)`, which is deprecated. The outer constructor calls `Base.depwarn`, and that walks a backtrace on every call, so models with an `arraydist` likelihood paid it once per evaluation. The return type is unchanged. diff --git a/Project.toml b/Project.toml index f8891bc3a..807a40e9f 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "DynamicPPL" uuid = "366bfd00-2699-11ea-058f-f148b4cae6d8" -version = "0.42.4" +version = "0.42.5" [deps] ADTypes = "47edcb42-4c32-4615-8424-f2b9edc5f35b" From 99b6471156ea29c618e60f5a7e44425f9e8bfe3c Mon Sep 17 00:00:00 2001 From: Shravan Goswami Date: Sun, 23 Aug 2026 04:25:41 +0530 Subject: [PATCH 3/6] Also test the logjac with a fixed transform that isn't the link The existing assertion uses `dist`'s own link transform as the target, so the target's forward Jacobian and the source's come out numerically identical and either would satisfy it. `other_ft` tells them apart. --- test/transformed_values.jl | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/test/transformed_values.jl b/test/transformed_values.jl index 8e5808e29..0dc8b993e 100644 --- a/test/transformed_values.jl +++ b/test/transformed_values.jl @@ -229,7 +229,7 @@ end dist = Beta(2, 2) vn = @varname(x) ft = FixedTransform(Bijectors.VectorBijectors.from_linked_vec(dist)) - wrong_ft = FixedTransform( + other_ft = FixedTransform( Bijectors.VectorBijectors.from_linked_vec(InverseGamma(2, 3)) ) @@ -254,10 +254,32 @@ end @test new_logjac ≈ logjac # Mismatched transform should error - strategy_bad = DynamicPPL.WithTransforms(VarNamedTuple(; x=wrong_ft), UnlinkAll()) + strategy_bad = DynamicPPL.WithTransforms(VarNamedTuple(; x=other_ft), UnlinkAll()) @test_throws ErrorException DynamicPPL.apply_transform_strategy( strategy_bad, tv, vn, dist ) + + # `other_ft` is not `dist`'s own link transform, so unlike the assertion above + # the target's forward log-Jacobian here is distinguishable from the source's. + @testset "log-Jacobian depends only on the target transform" begin + other_logjac = last( + Bijectors.with_logabsdet_jacobian( + Bijectors.inverse(other_ft.transform), raw_val + ), + ) + @test !isapprox(other_logjac, logjac) + + strategy = DynamicPPL.WithTransforms(VarNamedTuple(; x=other_ft), UnlinkAll()) + vec_val = Bijectors.VectorBijectors.to_vec(dist)(raw_val) + @testset "from $(get_transform(src))" for src in ( + TransformedValue(linked_val, DynamicLink()), + TransformedValue(vec_val, Unlink()), + TransformedValue(raw_val, NoTransform()), + ) + _, _, lj = DynamicPPL.apply_transform_strategy(strategy, src, vn, dist) + @test lj ≈ other_logjac + end + end end end From efea66587e596506a48cdfd33398c4b6b85d684d Mon Sep 17 00:00:00 2001 From: Shravan Goswami Date: Sun, 23 Aug 2026 04:26:54 +0530 Subject: [PATCH 4/6] Fix the transform table in the apply_transform_strategy docstring The table claimed a `FixedTransform` input errors against a `DynamicLink` or `Unlink` target. Both convert fine; only a mismatched `FixedTransform` target errors. It also had no column for `FixedTransform` targets, which is the path this branch changes. Spelling out that the log-Jacobian is the target's alone makes the contract the fix restores explicit. --- src/transformed_values.jl | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/transformed_values.jl b/src/transformed_values.jl index 36afed56b..a359497e4 100644 --- a/src/transformed_values.jl +++ b/src/transformed_values.jl @@ -303,20 +303,23 @@ Specifically, this function does a number of things: Otherwise, either linking or unlinking is applied as necessary. Note that this function does not perform vectorisation unless it is needed. - A table summarising the possible transformations is as follows: - - | tv.transform isa ...| `target_transform(...) isa DynamicLink` | `target_transform(...) isa Unlink` | - |---------------------|---------------------------------|------------------------------------| - | `DynamicLink` | -> `DynamicLink` | -> `NoTransform` | - | `Unlink` | -> `DynamicLink` | -> `Unlink` | - | `NoTransform` | -> `DynamicLink` | -> `NoTransform` | - | `FixedTransform` | errors | errors | - - Note that, for the last row, when using `FixedTransform` we require that `target_transform` - exactly matches the fixed transform, otherwise an error is thrown. - -- If `vn` is supposed to be linked, calculates the associated log-Jacobian adjustment for - the **forward** linking transformation (i.e., from unlinked to linked). + A table summarising the possible transformations is as follows, writing `target` for + `target_transform(strategy, vn)`: + + | `tv.transform` isa | target `DynamicLink` | target `Unlink` or `NoTransform` | target `FixedTransform` | + |--------------------|----------------------|----------------------------------|-------------------------| + | `DynamicLink` | -> `DynamicLink` | -> `NoTransform` | -> `FixedTransform` | + | `Unlink` | -> `DynamicLink` | -> `Unlink` | -> `FixedTransform` | + | `NoTransform` | -> `DynamicLink` | -> `NoTransform` | -> `FixedTransform` | + | `FixedTransform` | -> `DynamicLink` | -> `NoTransform` | -> `FixedTransform` | + + When `tv.transform` and `target` are both `FixedTransform`s, they must be equal, otherwise + an error is thrown. + +- Calculates the log-Jacobian adjustment for the **forward** transformation from the raw + value to the new internal representation (for instance, from unlinked to linked). This + depends only on `target`, never on `tv`'s current transform, and is zero when the new + representation is untransformed. This function returns a tuple of `(raw_value, new_tv, logjac)`. From 20e567a437d53b96502bf42bd0d03c37b6c68f09 Mon Sep 17 00:00:00 2001 From: Shravan Goswami Date: Sun, 23 Aug 2026 04:28:15 +0530 Subject: [PATCH 5/6] Check that fixing the link transforms keeps the same log-Jacobian Covers the bug where it counted: `InitFromParams` handing linked values to a fixed-transform strategy. The unit tests exercise `apply_transform_strategy` branch by branch, which is how the doubled Jacobian slipped through. --- test/contexts/init.jl | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/contexts/init.jl b/test/contexts/init.jl index baaf20170..d923115de 100644 --- a/test/contexts/init.jl +++ b/test/contexts/init.jl @@ -322,6 +322,23 @@ using Test ifp = InitFromParams(ps) @test ifp.params === ps.params end + + @testset "fixing the link transforms preserves the log-Jacobian" begin + @model function fixed_transform_model() + a ~ Exponential() + return b ~ Normal() + end + model = fixed_transform_model() + linked = last(DynamicPPL.init!!(model, VarInfo(), InitFromPrior(), LinkAll())) + # Feeding linked values into the equivalent fixed transforms must not change the + # log-Jacobian: it belongs to the target transforms, not to the representation + # the values arrived in. + strategy = WithTransforms(get_fixed_transforms(model, LinkAll()), LinkAll()) + _, vi = DynamicPPL.init!!( + model, VarInfo(), InitFromParams(linked.values), strategy + ) + @test DynamicPPL.getlogjac(vi) ≈ DynamicPPL.getlogjac(linked) + end end end From 47431f562fb6814b9748c23083b2f1214f1369e9 Mon Sep 17 00:00:00 2001 From: Shravan Goswami Date: Sun, 23 Aug 2026 04:28:48 +0530 Subject: [PATCH 6/6] Say which Jacobian was wrong in the changelog entry --- HISTORY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/HISTORY.md b/HISTORY.md index 5d4cf6d5e..ee680fea1 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,6 +1,6 @@ # 0.42.5 -Fixed `apply_transform_strategy` to accumulate only the target fixed transform's forward log-Jacobian when converting from `DynamicLink`. Previously, the source inverse Jacobian was also included, producing incorrect internal log densities. +Fixed `apply_transform_strategy` to return only the target transform's forward log-Jacobian when converting a `DynamicLink` value to a `FixedTransform` target. It previously added the source's forward Jacobian as well, so `getlogjac`, `getlogjoint_internal`, and `getlogprior_internal` were wrong whenever linked values were fed into a fixed-transform strategy. See [#1407](https://github.com/TuringLang/DynamicPPL.jl/issues/1407). # 0.42.4