-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add potentials to priors when running abc #4016
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4016 +/- ##
=======================================
Coverage 86.77% 86.77%
=======================================
Files 88 88
Lines 14137 14144 +7
=======================================
+ Hits 12267 12274 +7
Misses 1870 1870
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aloctavodia, LGTM! I also added @junpenglao and @lucianopaz as reviewers, as I think they know the context around this better than me.
|
||
if self.kernel.lower() == "abc": | ||
factors = [var.logpt for var in self.model.free_RVs] | ||
factors += [tt.sum(factor) for factor in self.model.potentials] | ||
self.prior_logp_func = logp_forw([tt.sum(factors)], self.variables, shared) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused. Why doesn't model.logpt
work? To me, it looks like the only difference is that the observed_RV
s aren't included in your factors whereas they are in the model.logpt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SMC needs to evaluate the prior and likelihood terms idenpendently, the sampler progressively turns on the likelihood term. Additionally in ABC, there is no true likelihood. The likelihood is approximated as a discrepancy between observed and simulated data. That's why I am moving the potential to the prior, only for this type of model/sampler
This works fine for Potential as a constrain to the prior, but not if the Potential is part of the likelihood term (those you need to add to the likelihood func). Unless there is a way to detect the dependency and add the potential to different term, this change is not safe/correct. The model that would be incorrect is something like: with pm.Model() as SMABC_potential_liklihood:
a = pm.Normal("a", mu=0, sigma=1)
b = pm.HalfNormal("b", sigma=1)
c = pm.Potential("c", pm.math.switch(a > 0, 0, -np.inf)) # <== this should be added to prior
s = pm.Simulator(
"s", normal_sim, params=(a, b), sum_stat="sort", epsilon=1, observed=data
)
pm.Potential("s_p", pm.Normal.dist().logp(s)) # <== this should be added to likelihood |
This change only affects SMC-ABC, i.e. models without a likelihood. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change only affects SMC-ABC, i.e. models without a likelihood.
Could you add a warning if there are Potentials in the model to indicate that they will be added to the prior? I dont think it is a common use case for user to write down model like the one I wrote above, but currently we dont forbid ppl to write those kind of model so we might as well raise a warning.
Thanks @aloctavodia! |
Because potentials are applied to the likelihood term they are invisible to ABC models. This change it (only for ABC).
Sampling from the prior still is still unaware of potentials, this not fix that as it should be solved in the sampling_prior_predictive function. Unless we decide not to provide a general solution (in that case I will solve it for SMC and SMC/ABC)