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 generates incorrect-shape samples for DensityDist #3553

Closed
rpgoldman opened this issue Jul 18, 2019 · 5 comments
Closed
Assignees

Comments

@rpgoldman
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

rpgoldman commented Jul 18, 2019

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
Copy link
Contributor

lucianopaz commented Jul 19, 2019

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

@lucianopaz
Copy link
Contributor

@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
Copy link
Contributor

This is fixed by #3554

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

No branches or pull requests

2 participants