Error informatively on partial specification of a multivariate variable (#2239)#1434
Error informatively on partial specification of a multivariate variable (#2239)#1434yebai wants to merge 9 commits into
Conversation
A variable drawn from a multivariate distribution in a single tilde-statement (e.g. `x ~ MvNormal(...)` / `filldist`) is a single random variable, not i.i.d. components. `predict` therefore cannot fix a subset of its components while resampling the rest, and `fix` cannot fix them independently. Add warning admonitions documenting this, referencing TuringLang/Turing.jl#2239. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1434 +/- ##
==========================================
+ Coverage 81.64% 81.69% +0.05%
==========================================
Files 50 50
Lines 3579 3595 +16
==========================================
+ Hits 2922 2937 +15
- Misses 657 658 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #1434 is available at: |
Benchmarks @ 4a9e25aPerformance 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 @ d7e84ceEnvironmentJulia 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) |
…s specified Supplying a subset of a variable that the model samples as a single multivariate draw (e.g. `x[1:10]` when the model draws `x ~ MvNormal(zeros(20), ...)`) has never worked: `predict` silently resampled the whole variable from the prior, `fix` silently collapsed it to the supplied length, and `condition` threw an opaque `DimensionMismatch`. This is therefore not a behaviour change — it only replaces those silent or confusing failures with a single, informative error. Add `_check_supplied_shape(dist, supplied, vn)`, dispatched on the supplied representation (a materialised value for `condition`/`fix`, or the parameter `VarNamedTuple` for `predict`/`InitFromParams`), which throws one clear error referencing TuringLang/Turing.jl#2239. Only multivariate distributions are checked; per-index (`x[i] ~ ...`) declarations and correctly-sized whole-variable values are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the #2239 misuse cases: predicting with a chain from a differently-sized model, and `fix`/`condition` of a single index of a multivariate variable, all now raise the informative error. A whole-variable `fix` of the correct size is kept as a positive control. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No behaviour change. Shorten the `_check_supplied_shape` docstring and the `init` comment, and reduce the predict misuse test to the smallest model that still triggers the error (just the multivariate variable, no extra latent or observation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the new informative error for partial specification of a multivariate variable via condition/fix/predict (Turing#2239). Non-breaking: the case never worked, so this only replaces a silent or opaque failure with a clear message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The docstring notes previously described the old silent behaviour (predict resampling the whole variable from the prior); update them to state that supplying only part of a multivariate variable now raises an error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
predict and fixCo-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the error self-contained and actionable (the loop workaround); the #2239 reference stays in the docstrings and HISTORY. Tests now match on a stable phrase from the message rather than the issue number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback: raise `ArgumentError` (consistent with `check_tilde_rhs` /`check_dot_tilde_rhs`) instead of a generic `error`, and match it by type in the tests rather than by a fragile message substring. Add a regression test that per-index (`.~`) variables still grow correctly under `predict` without being falsely flagged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@sunxd3, can you help review this? |
|
Yes! |
|
I'm not sure the macro-level guard is the right place for this check — it both misses cases and catches one it shouldn't. It only runs when On the other side, a conditioned value is an observation, and the observe pipeline accepts batches via @model mvc(n) = m ~ MvNormal(zeros(n), I)
# errors on this branch:
fix(mvc(3), Dict(@varname(m[1]) => 1.0))()
# silently ignored, m sampled from the prior:
fix(mvc(3), Dict(@varname(m[1]) => 1.0, @varname(m[3]) => 3.0))()
# two draws, summed by loglikelihood — works on main, errors on this branch:
loglikelihood(condition(mvc(3), (; m = [1.0 2.0; 3.0 4.0; 5.0 6.0])), (;))Getting condition/fix right would need the tilde pipeline to tell "absent" apart from "partially present", and to treat observe-side shapes differently from assume-side — that's on the hot path. Honestly not sure it's worth fixing at all; if it is, definitely its own PR. So maybe we scope this down: keep the PR documentation-only (note the whole-variable semantics in the (prepared with CC + Fable) |
|
Thanks, @sunxd3. It would be great to find a robust, principled solution that catches unintended uses of One approach that often works well is to brainstorm with coding agents: give them a clear design goal or constraint, ask them to explore feasible solutions, and then evaluate the trade-offs of their proposals. In a sense, the process resembles the Metropolis–Hastings accept/reject step in MCMC, with humans deciding which proposed solutions to keep. I'm happy to merge the docstring fixes first, as you proposed, and then have you take on the remaining changes in a separate PR. |
Fixes the documentation and the behaviour side of TuringLang/Turing.jl#2239.
A variable drawn from a multivariate distribution in a single tilde-statement (e.g.
x ~ MvNormal(...),filldist) is a single random variable, not a collection of i.i.d. components. Supplying only part of such a variable — viapredictwith a chain from a differently sized model, orfix/conditionon a subset of indices — should produce an informative error.