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

User-defined distributions #143

Closed
jpchen opened this issue Sep 22, 2017 · 7 comments
Closed

User-defined distributions #143

jpchen opened this issue Sep 22, 2017 · 7 comments

Comments

@jpchen
Copy link
Member

jpchen commented Sep 22, 2017

We should allow users to define their own distributions. Came across this problem when specifying priors over parameters, where the prior needs to take the shape of the parameter to sample properly. This prior needs to have the same interface as a pyro distribution because the trace methods assumes certain characteristics of a distribution object that a function does not have.

I was imagining something like:

def sample_fn(*args, **kwargs):
  //sample

dist = pyro.distributions.customDistribution({
  'init': init_fn,
  'sample': sample_fn,
  'log_pdf': log_pdf_fn,
  'batch_log_pdf' (optional): ...,
  'support' (optional): ...,
})

or have setter methods to do this. Something along those lines

@eb8680
Copy link
Member

eb8680 commented Sep 22, 2017

I'd prefer not to encourage users to dynamically generate distributions with new and untested samplers and scorers. I think maybe a better solution to your particular problem is to write a class/function TensorDistribution that takes distributions with independent elements (e.g. DiagNormal) and turns them into distributions over tensors of arbitrary (fixed?) shape, then have the prior in LiftPoutine construct and return such a distribution object instead of a sample and modify LiftPoutine accordingly. You might even be able to use TransformedDistribution for this?

If users really need custom distributions, they can write a new Distribution class.

@fritzo
Copy link
Member

fritzo commented Sep 22, 2017

It would be really nice to have extensible distributions, independent of Pyro. It seems like there are two very different approaches:

  • Composable: we make it easy to create complex distribution objects from basic distributions. This is @eb8680 's approach.
  • Easily extensible: we make it easy for users to create a new Distribution object, either by inheritance or by @jpchen 's customDistribution(). To make this easy, we'd also provide generic test utilities that could check agreement among sample, log_pdf, and batch_log_pdf. This was the purpose of posterior/goftests, but it was still a lot of work to create a new distribution in posterior/distributions. My last team nimble-dev/nimble allows extension, but doesn't provide generic test utilities to check agreement between sample() and logprob().

@eb8680
Copy link
Member

eb8680 commented Sep 22, 2017

Just to be clear, there's not much difference between writing a custom distribution class that inherits from our base Distribution and doing what @jpchen proposed - it's more about what idioms we should encourage.

@jpchen
Copy link
Member Author

jpchen commented Sep 22, 2017

I think maybe a better solution to your particular problem is to ...

yes that's exactly what i did locally, since i only had a few, but i was just thinking in the case in which one had many different shapes/priors that required their own distribution if we want them to create classes for each.

@fritzo yes i agree, i think it's just about what the "pyronic" way to do this is, as @eb8680 noted above. if we were to provide a custom class for them to do this, we'd probably have to do some safety checking like that, and also setting class properties correctly. though it's on the user to make sure their sample and score functions are parameterized correctly. also, they probably wouldnt be able to call the functional form of the distribution as you currently can do for all the provided distributions.

@fritzo
Copy link
Member

fritzo commented Sep 22, 2017

@neerajprad If we intend to allow users to create custom distributions, it would be a good idea for us to move generic testing tools from testing/test_distributions.py into a pyro.testing module that ships via pip install pyro. See also #120

@neerajprad
Copy link
Member

+1. We should have that so that users can run some tests on the custom defined distribution, to see if the sampling and scoring are coherent, for instance. Will keep that in mind.

@ngoodman
Copy link
Collaborator

i love the idea of providing sample/score coherence tests. maybe even providing them in such a way that if you extend Distribution they get automatically added to the tests.

i'm more agnostic about pyronic way to make distributions, but generally would prefer to default to the pythonic way, which seems to be to make a custom class extending Distribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants