sample_posterior_predictive generates incorrect-shape samples for DensityDist

[rpgoldman]

If you have questions about a specific use case, or you are not sure whether this is a bug or not, please post it to our discourse channel: https://discourse.pymc.io

Description of your problem

When generating posterior predictive samples, we try to generate samples for ObservedRVs that match the shape of the observations. This fails for DensityDist.

Example

In #3552 I revised tests/test_distributions_random.py::test_density_dist_with_random_sampleable to check the shape of the values for the observed random variable, instead of just the length of the trace.

So if you check out the change in that PR and run

pytest pymc3/tests/test_distributions_random.py::test_density_dist_with_random_sampleable

... you will see the error.

Additional Information

You can see the issue in this test:

• We let the user provide an arbitrary random function for DensityDist:

  pm.DensityDist('density_dist', normal_dist.logp, observed=np.random.randn(observations), random=normal_dist.random)

• Contrast this with the random function for the Normal distribution:

    mu, tau, _ = draw_values([self.mu, self.tau, self.sigma],
                             point=point, size=size)
    return generate_samples(stats.norm.rvs, loc=mu, scale=tau**-0.5,
                            dist_shape=self.shape,
                            size=size)
  

The difference is that the DensityDist has no built in mechanism to force the supplied random function to honor the shape attribute of the distribution. So, as this test shows, the shape is ignored, which is not the same thing as happens with other ObservedRVs.

As the PR shows, this test should never have been considered as passing for the current definition of sample_posterior_predictive().

Suggested solutions

1. Require the supplied random function to take a shape keyword argument, pass shape=self.shape to it from DensityDist.random(), and trust users to Do The Right Thing. Expand the docstring.

2. Disable posterior predictive sampling for DensityDist. This seems undesirable.

Versions and main components

• PyMC3 Version: master as of today
• Other components: irrelevant

[lucianopaz]

Oh, this is a hard one. I think that we can only aim to wrap the random method to check that the distribution's shape matches the shapes of the drawn samples and then raise an exception otherwise. We should document it and state that the users must take great care in cases where their DensityDists are related to shared tensor's

[rpgoldman]

With this check in place, I am now seeing problems with pymc3/tests/test_data_container.py::TestData::test_sample (see https://travis-ci.org/pymc-devs/pymc3/jobs/560688374).

For some reason, adding a shape check is causing this test to fail. @lucianopaz -- would you please look at this? I believe my shape error checking is colliding with the structure of _draw_value using exception-handling to probe a variable's shape. This may also be a Heisenbug, because when I throw myself into the debugger, I may not be seeing the same state as the state of the branching.

Also, @lucianopaz, did I misinterpret your suggestion? I was checking all of random calls in _draw_sample -- did you instead just want me to do the check in DensityDist.random()?

[lucianopaz]

@rpgoldman I hadn't seen your last comment before writing #3554, which only addresses the DensityDist stuff.

[lucianopaz]

@rpgoldman, I just looked at the exception you're seeing and I can confirm it is not a bug. It is related to the fixed issue #3147. Basically, a distribution has its shape, the distribution's parameters have their shape, and the samples returned by the distribution's random broadcast both the parameters' shape and the distribution's own shape. It is part of our very own shell (shape hell). I have to write some proper documentation about this kind of stuff eventually.

[lucianopaz]

This is fixed by #3554