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

DOC Add Predictive examples #1084

Merged
merged 8 commits into from
Jul 11, 2021
Merged

Conversation

MarcoGorelli
Copy link
Contributor

@MarcoGorelli MarcoGorelli commented Jul 4, 2021

xref #1082 (comment)

I thought it might be clearer to slightly edit the existing examples in SVI and MCMCKernel, which already have runnable code, and then to refer to them in the Predictive kernel

Here's the part added to Predictive:

image

SVI:

image

MCMCKernel:

image

MCMC.run:

image


EDIT

some doctest failure...I'll fix this later, the example in numpyro/infer/util.py needs to just be code, but without doctest running on it

@MarcoGorelli MarcoGorelli changed the title document Predictive DOC Add Predictive examples Jul 4, 2021
@MarcoGorelli MarcoGorelli force-pushed the document-predictive branch from a9b6e90 to 65e858b Compare July 4, 2021 17:44
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @MarcoGorelli! I think you can remove >>> to skip doctest.

Given a model, you can sample from the prior predictive:

>>> predictive = Predictive(model, num_samples=num_samples)
>>> samples = predictive(rng_key1, *model_args, **model_kwargs)
Copy link
Member

@fehiepsi fehiepsi Jul 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rng_key1 -> rng_key

To make it clearer that we need to set data=None, I think you can provide a signature for model:

Given a model

    def model(X, y=None):
        ...
        return numpyro.sample("obs", likelihood, obs=y)

you can sample from the prior predictive:

    predictive = Predictive(model, num_samples=1000)
    y_pred = predictive(rng_key, X)["obs"]

If you also have posterior samples, you can sample from the posterior predictive:

    predictive = Predictive(model, posterior_samples=posterior_samples)
    y_pred = predictive(rng_key, X)["obs"]

@@ -101,6 +101,8 @@ class SVI(object):
>>> svi = SVI(model, guide, optimizer, loss=Trace_ELBO())
>>> svi_result = svi.run(random.PRNGKey(0), 2000, data)
>>> params = svi_result.params
>>> predictive = Predictive(model, guide=guide, params=params, num_samples=1000)
>>> samples = predictive(random.PRNGKey(0), data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not return any useful information unless you set data=None (which will give you posterior predictive samples). If you want to get posterior samples, then you can move those codes to the end and add a comment

>>> # get posterior samples
>>> predictive = Predictive(guide, params=params, num_samples=1000)
>>> samples = predictive(random.PRNGKey(1), data)

>>> samples = mcmc.get_samples()
>>> posterior_samples = mcmc.get_samples()
>>> predictive = Predictive(model, posterior_samples=posterior_samples)
>>> samples = predictive(rng_key1, *model_args, **model_kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have a model here. I think you can add a small code snippet to get_samples method of MCMC.

@@ -509,6 +511,8 @@ def run(self, rng_key, *args, extra_fields=(), init_params=None, **kwargs):
"""
Run the MCMC samplers and collect samples.

See :class:`~numpyro.infer.util.Predictive` for how to use them to collect posterior predictive samples.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think get_samples is a better place for this. Also maybe changing them to those samples?


Given a model, you can sample from the prior predictive:

>>> predictive = Predictive(model, num_samples=num_samples)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit num_samples=1000

@fehiepsi fehiepsi mentioned this pull request Jul 5, 2021
8 tasks
@MarcoGorelli
Copy link
Contributor Author

Thanks for your feedback! I'll address your comments at the weekend (won't have time this week before then unfortunately)

@fehiepsi
Copy link
Member

fehiepsi commented Jul 7, 2021

Please take your time. We can always defer doc improvements to later releases. :)

@fehiepsi fehiepsi added the WIP label Jul 8, 2021

posterior_samples = mcmc.get_samples()
predictive = Predictive(model, posterior_samples=posterior_samples)
samples = predictive(rng_key1, *model_args, **model_kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need an empty string after this to make lint pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be the * - will try making it a r string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fixed, it just needed to be put into a code block, so with the previous line ending with :: rather than :

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it looks like this:

Screenshot 2021-07-11 at 21 17 21

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good catch!

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @MarcoGorelli!

@fehiepsi fehiepsi merged commit cf85229 into pyro-ppl:master Jul 11, 2021
@MarcoGorelli MarcoGorelli deleted the document-predictive branch July 11, 2021 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants