Predictive fix when deterministic sites are present #1789
Conversation
kylejcaron
changed the title
numpyro/infer/util.py
Outdated
| model_trace = trace( | ||
| seed(substitute(masked_model, samples), rng_key) | ||
| seed( | ||
| substitute(masked_model, substitute_fn=_samples_wo_deterministic), |
There was a problem hiding this comment.
Could you help me change line 777 to condition so that? It would be nice to add an argument like exclude_deterministic_from_posterior to Predictive to maintain two behaviors. We will pass such argument to this _predictive function to control the behavior.
There was a problem hiding this comment.
I added the conditional logic, I tried the name exclude_deterministic_params because it was shorter but let me know if that's insufficient
For this could you elaborate?
Could you help me change line 777 to condition so that?
Would I be changing L777 from substitute to condition?
And should I add the deterministic fix there as well?
There was a problem hiding this comment.
Yup, using condition there is fine because we don't substitute deterministic sites under thecondition handler.
There was a problem hiding this comment.
ok just switched it to condition
There was a problem hiding this comment.
Thanks for fixing the breakage, @kylejcaron! We'll make a patch release soon.
* added custom effect handler for predictive * added test, fixed predictive_substitute * fixed typo, removed unneeded custom substitute calls * removed custom effect handler, improved readability * reverted formatting of imports * added conditional arg for handling deterministic sites to predictive * changed arg name to exclude_deterministic * updated exclude_deterministic description * changed substitute to condition in infer_discrete _predctive workflow --------- Co-authored-by: kylejcaron <[email protected]>
This PR attempts to fix #1772 - when deterministic sites are included in
Predictivewith params,Predictivewon't generate new samples for those deterministic sites.This PR solves that by ignoring deterministic sites in the Predictive
substitutecall. Added a new test to cover this scenario as well.