Rename Predictive's exclude_deterministic to condition_deterministic - #2259
Rename Predictive's exclude_deterministic to condition_deterministic#2259Agi-Asi wants to merge 1 commit into
Conversation
Issue pyro-ppl#2086 reported that Predictive.exclude_deterministic reads as if it controls which sites are returned, when it actually controls whether deterministic sites in posterior_samples are conditioned on during substitution. Maintainers converged on the clearer name condition_deterministic (suggested by kylejcaron, endorsed by juanitorduz). - new keyword condition_deterministic (default False, same behaviour as the old default exclude_deterministic=True) - exclude_deterministic kwarg still accepted with a FutureWarning; passing both raises ValueError - exclude_deterministic attribute kept as a deprecated property (getter and setter) so existing code that flips it keeps working - docstring now explains what the flag actually does, points to return_sites for output filtering, and documents why conditioning on deterministic sites is off by default (shape/staleness bugs, see pyro-ppl#1772) - tests for the deprecation path, both-kwargs error, and default Closes pyro-ppl#2086
Benchmark reportthis PR - run time: 1 slower, 1 faster
compile time: unchanged across 32 benchmarksSignificant changes (2) ─────── run time ─────── ────── compile time ─────
benchmark baseline this PR Δ baseline this PR Δ
───────────────────────────────────────────────────────────────────────────
+ dirichlet_sample 50.0 ms 45.8 ms -8.5% 854.8 ms 884.1 ms +3.4%
- normal_sample 22.3 ms 23.5 ms +5.4% 209.4 ms 209.3 ms -0.0%Red is slower, green is faster; a row is coloured by the worse of its two columns. A delta in parentheses cleared the threshold on a measurement below the resolution floor, so it is shown without being called a change. † marks a benchmark that could not be compared — see below. Full results
|
| baseline | this PR | |
|---|---|---|
| ref | master |
feat/condition-deterministic-rename |
| commit | 0059d3af |
ece8672c |
| numpyro | 0.21.0 | 0.21.0 |
| jax | 0.11.1 | 0.11.1 |
| backend | cpu | cpu |
| python | 3.14.7 | 3.14.7 |
Runner: Linux-6.17.0-1022-azure-x86_64-with-glibc2.39, 4 CPUs.
Produced by this benchmark run.
juanitorduz
left a comment
There was a problem hiding this comment.
I like the suggestion :) It is clearer
Implements the rename discussed in #2086:
Predictive'sexclude_deterministic→condition_deterministic(name suggested by @kylejcaron, endorsed by @juanitorduz).Why: the old name reads as if it filters deterministic sites out of the returned dictionary, but it actually controls whether deterministic sites in
posterior_samplesare conditioned on during substitution — the returned sites are governed byreturn_sites. That mismatch is exactly what confused the issue reporter (and, per the thread, the argument's own author).What this PR does:
condition_deterministic: bool = False— the default is behaviour-identical to the oldexclude_deterministic=Truedefault, so nothing changes for existing users.exclude_deterministic=is still accepted (mapped tonot condition_deterministic) with aFutureWarning; passing both raisesValueError. Thepredictive.exclude_deterministicattribute is kept as a deprecated property (getter + setter) so code that flips it after construction keeps working.return_sitesfor output filtering, and explain why conditioning on deterministic sites is off by default (shape/staleness bugs when predicting on new data — see numpyro.deterministic static on infer.Predictive #1772).ValueError, default value, and the existing discrete-predictive test migrated to the new name.Note:
MCMC.print_summary(exclude_deterministic=...)is a different API where the name is accurate (it filters printed output), so it is deliberately untouched.ruff check,ruff format --check, andty checkall pass; relevant test selection (18 predictive tests) passes locally.Closes #2086