Skip to content
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

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

aloctavodia
Copy link
Member

@aloctavodia aloctavodia commented Jul 16, 2020

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)

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #4016 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4016   +/-   ##
=======================================
  Coverage   86.77%   86.77%           
=======================================
  Files          88       88           
  Lines       14137    14144    +7     
=======================================
+ Hits        12267    12274    +7     
  Misses       1870     1870           
Impacted Files Coverage Δ
pymc3/smc/sample_smc.py 87.95% <100.00%> (+0.45%) ⬆️
pymc3/smc/smc.py 99.49% <100.00%> (+0.01%) ⬆️
pymc3/gp/cov.py 97.96% <0.00%> (ø)

Copy link
Contributor

@AlexAndorra AlexAndorra left a 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)
Copy link
Contributor

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_RVs aren't included in your factors whereas they are in the model.logpt.

Copy link
Member Author

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

@junpenglao
Copy link
Member

junpenglao commented Jul 17, 2020

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

@aloctavodia
Copy link
Member Author

This change only affects SMC-ABC, i.e. models without a likelihood.

Copy link
Member

@junpenglao junpenglao left a 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.

@twiecki twiecki merged commit 611eceb into pymc-devs:master Jul 27, 2020
@twiecki
Copy link
Member

twiecki commented Jul 27, 2020

Thanks @aloctavodia!

@aloctavodia aloctavodia deleted the smc_potentials branch July 27, 2020 18:56
@kyleabeauchamp kyleabeauchamp added this to the 3.9.3 milestone Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants