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

Add FunsorDistribution to wrap funsors for use in Pyro #170

Merged
merged 12 commits into from
Jul 24, 2019

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Jul 24, 2019

Addresses #177

This adds a basic FunsorDistribution class inheriting from TorchDistribution. The class is intended to wrap a Funsor as a Distribution object for use in Pyro.

I've created a new module funsor.pyro and I plan to create new submodules like funsor.pyro.hmm #171 containing various hidden-markov models (discrete, gaussian, slds, ...). Each of these distributions will use fancy funsor syntax internally, but will expose their inference algorithms as black-box Distributions with .sample() and .log_prob() methods. These distributions should be fully compatible with broadcasting, plates, and enumeration dims (which should pass right through these algorithms).

Tested

  • added unit tests for conversion helpers
  • added a basic test creating a Categorical distribution from a funsor

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

LGTM, I just had a couple questions about followup PRs.

return value

def expand(self, batch_shape, _instance=None):
raise NotImplementedError("TODO")
Copy link
Member

Choose a reason for hiding this comment

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

.expand will be necessary for most Pyro use cases, right? Are you planning to implement this generically in a followup PR, or case-by-case for specific FunsorDistribution subclasses? Also, can we use funsor.Independent to implement FunsorDistribution.to_event?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm planning to implement .expand() generically when I eventually need it.

Hmm yes, I think we might be able to use funsor.Independent for .to_event().

return tensor


def dist_to_funsor(pyro_dist, reinterpreted_batch_ndims=0):
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to implement dist_to_funsor generically and use it to replace the current funsor.distributions as suggested in #159, with a few extra eager patterns for Categorical, Normal/MultivariateNormal, and Delta

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, yes. In practice, there is a mismatch between the dtype and shapes in funsor versus distributions. I welcome any effort to implement this generically, but I suspect every distribution will be a new edge case.

.travis.yml Outdated Show resolved Hide resolved
@fritzo
Copy link
Member Author

fritzo commented Jul 24, 2019

Thanks for reviewing!

@fritzo fritzo merged commit 7c0b068 into master Jul 24, 2019
@fritzo fritzo deleted the pyro-distributions branch August 7, 2019 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants