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

sample_posterior_predictive flattens chains and draws #4004

Closed
kyleabeauchamp opened this issue Jul 6, 2020 · 13 comments · Fixed by #4006
Closed

sample_posterior_predictive flattens chains and draws #4004

kyleabeauchamp opened this issue Jul 6, 2020 · 13 comments · Fixed by #4006
Assignees

Comments

@kyleabeauchamp
Copy link
Contributor

I noticed that the results of pm.sample_posterior_predictive are flattened over (chain, draw) dimensions. AFAIK, this is a lossy transform because the results are numpy objects that don't track the source. I wonder if there should be an option to preserve the (chain, draw) shape and output the results using an arviz InferenceData object. See also arviz-devs/arviz#1282

@rpgoldman
Copy link
Contributor

I believe that there are already options to do this. Have you tried using the keep_size argument to sample_posterior_predictive? Also, ArviZ has a function that translates a PyMC3 set of predictive samples into an InferenceData. Do either of those do what you want?

@kyleabeauchamp
Copy link
Contributor Author

In my test case, I had 2 chains and 500 draws per chain. With keep_size=False, the output of PPC was 1000. With keep_size=True, the output of PPC was (1000, 1).

I'll look at from_pymc3_predictions(), as that looks like exactly what I needed!

@kyleabeauchamp
Copy link
Contributor Author

Here is my minimal reproducing example:

import numpy as np
import pymc3 as pm

chains = 2
n_samples = 1000
n_things = 25

y = np.random.normal(size=n_things) + 5.0

coords = {"things": range(n_things)}

with pm.Model(coords=coords) as model:
    y = pm.Data('y', y, dims="things")
    mu = pm.Normal('mu', 0, sd=50.0)
    obs = pm.Normal('obs', mu, observed=y, dims="things")
    tr = pm.sample(n_samples, chains=chains, return_inferencedata=True)

ppc_true = pm.sample_posterior_predictive(tr.posterior, keep_size=True, model=model)
ppc_false = pm.sample_posterior_predictive(tr.posterior, keep_size=False, model=model)

print(tr.posterior.coords)
print(ppc_true["obs"].shape)
print(ppc_false["obs"].shape)


Coordinates:
  * chain    (chain) int64 0 1
  * draw     (draw) int64 0 1 2 3 4 5 6 7 8 ... 992 993 994 995 996 997 998 999
(1, 2000, 25)
(2000, 25)

@OriolAbril
Copy link
Member

Here are the relevant lines in sample_posterior_predictive, not sure if the same issue happens is fast_sample_posterior_predictive.

https://github.com/pymc-devs/pymc3/blob/master/pymc3/sampling.py#L1598-L1605

The chain information is not retrieved in the dataset case, only if a trace is passed, thus keep_size does not work if the input is a dataset. In the dataset case, doing nchain = trace.dims["chain"] should work

@rpgoldman
Copy link
Contributor

@kyleabeauchamp Thanks for checking this: it looks like keep_size does not work in sample_posterior_predictive. Would you mind checking to see if it is also broken in fast_sample_posterior_predictive?

I can shove these into the tests, so that we can fix and make sure that keep_size continues to work correctly.

@kyleabeauchamp
Copy link
Contributor Author

For the case of an arviz trace, fast_sample_posterior_predictive doesn't run at all due to some strong type checking:

lib/python3.7/site-packages/pymc3/distributions/posterior_predictive.py in fast_sample_posterior_predictive(trace, samples, model, var_names, keep_size, random_seed)
    182         if keep_size and not isinstance(trace, MultiTrace):
    183             # arguably this should be just a warning.
--> 184             raise IncorrectArgumentsError("keep_size argument only applies when sampling from MultiTrace.")
    185 
    186         if isinstance(trace, list) and all((isinstance(x, dict) for x in trace)):

IncorrectArgumentsError: keep_size argument only applies when sampling from MultiTrace.

For the case of a pymc3 trace, fast_sample_posterior_predictive does seem to respect the (chain, draw) shape:

import numpy as np
import pymc3 as pm

chains = 2
n_samples = 1000
n_things = 25

y = np.random.normal(size=n_things) + 5.0

coords = {"things": range(n_things)}

with pm.Model(coords=coords) as model:
    y = pm.Data('y', y, dims="things")
    mu = pm.Normal('mu', 0, sd=50.0)
    obs = pm.Normal('obs', mu, observed=y, dims="things")
    tr = pm.sample(n_samples, chains=chains)

ppc_true = pm.fast_sample_posterior_predictive(tr, keep_size=True, model=model)
ppc_false = pm.fast_sample_posterior_predictive(tr, keep_size=False, model=model)

print(ppc_true["obs"].shape)
print(ppc_false["obs"].shape)

(2, 1000, 25)
(2000, 25)


@OriolAbril
Copy link
Member

sample_posterior_predictive also respects keep_size when used with a pm.MultiTrace I remember adding tests for this to both PyMC3 and ArviZ

@rpgoldman
Copy link
Contributor

rpgoldman commented Jul 7, 2020

Probably we didn't get the handling of shape right when we added InferenceData input to fast_sample_posterior_predictive. I'll see about fixing this.

I would imagine that if we take an InferenceData as input, maybe we should return one as output.

@rpgoldman
Copy link
Contributor

@michaelosthege while I was debugging this, I was surprised to see that the posterior predictive sampling functions now accept an xarray dataset, but not an InferenceData object.
Shouldn't we permit an InferenceData also as an input, and then extract the posterior group ourselves for the default case?

@kyleabeauchamp
Copy link
Contributor Author

I also experienced this confusion---in the pymc3 codepath you pass the output of tr = pm.sample() but in the arviz codepath you must pass the the tr.posterior variable on the object...

@rpgoldman
Copy link
Contributor

@kyleabeauchamp I'm going to address this in my fix for this issue; I agree with you -- it seems odd to require that extra step if using InferenceData.

@rpgoldman rpgoldman self-assigned this Jul 9, 2020
@rpgoldman
Copy link
Contributor

Just debugging a solution to this now...

@rpgoldman
Copy link
Contributor

I have a WIP solution -- waiting to see if it passes CI.

@rpgoldman rpgoldman linked a pull request Jul 9, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants