Fix Jacobian for dynamic-to-fixed transforms - #1442
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1442 +/- ##
==========================================
+ Coverage 81.70% 81.84% +0.13%
==========================================
Files 50 50
Lines 3581 3581
==========================================
+ Hits 2926 2931 +5
+ Misses 655 650 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #1442 is available at: |
Benchmarks @ 47431f5Performance Ratio: gradient time divided by log-density time. For very small models these ratios are noisy across runs and machines; raw primal and gradient timings are more reliable. The benchmarks are aimed at DynamicPPL developers and mainly catch obvious allocation or type-stability regressions. See benchmark notes for details. Main @ a8b0145EnvironmentJulia Version 1.11.9 Commit 53a02c0720c (2026-02-06 00:27 UTC) Build Info: Official https://julialang.org/ release Platform Info: OS: Linux (x86_64-linux-gnu) CPU: 4 × AMD EPYC 7763 64-Core Processor WORD_SIZE: 64 LLVM: libLLVM-16.0.6 (ORCJIT, znver3) Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores) |
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.
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.
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.
|
@yebai Ready to merge, I will release it if you allow merging! |
|
It's better to have a look from a fresh perspective. @sunxd3 |
|
Sorry it took me a while, chasing a ghost that's turned out to be nothing. The changes here look good to me. Thanks for working on this! |
Return target forward Jacobians, add a DynamicLink-to-FixedTransform regression test
Fix #1407