-
Notifications
You must be signed in to change notification settings - Fork 20
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 optional input arguments to to_funsor and to_data #316
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fritzo
reviewed
Feb 12, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see a Funsor replacement for pyro.ops.packed!
- I assume you'll add tests after sketching the Pyro integration? Or could you point out how the
dim_to_name
kwarg is exercised by existing tests (sorry I don't see immediately). - Do you know whether the reshaping logic is NumPy compatible? If not, consider adding a TODO. No need to support both backends in this PR.
Thanks for the clear PR description! |
This was referenced Feb 13, 2020
Closed
eb8680
commented
Feb 13, 2020
fritzo
approved these changes
Feb 18, 2020
Sorry I missed this last week. Feel free to "bump" if I take longer than a couple days to review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Based on our design discussion on Monday, this PR is a first step toward replacing much of Pyro's internals with Funsor and supporting enumeration in NumPyro. It adds a
dim_to_name
mapping argument toto_funsor
and aname_to_dim
mapping argument toto_data
which provide sufficient information for uniquely converting PyTorch tensors and distributions to Funsors with free variables and vice versa.The changes in this PR only support
Tensor
s. In followup PRs, I will implement versions ofto_funsor
andto_data
that apply toDistribution
s.This code is ready for review, but before changing the design too much I'd like to have first versions of the plate/enum-aware wrappers (discussed below) implemented as well (update: pyro-ppl/pyro#2307)
Design
dim_to_name
is a dictionary that maps batch dimensions to (name, domain) pairs.name_to_dim
is a dictionary that maps input names to batch dimensions.This extra information (beyond an ordered list of names) is necessary for unique conversion because in Pyro we often have empty batch dimensions in our unpacked tensor shapes that do not mean anything, and eventually in Funsor we will support non-vector
bint
domains.These arguments are optional, and
to_funsor
andto_data
behave the same as before when they are not provided.Rationale
The primary use case for this new functionality is upstream in Pyro and NumPyro, where we would like to convert distributions and lazy sample values to and from funsors with shapes consistent with the global plate and enumeration dimension state. This code could plausibly live in Pyro, but I've put it here since it is intended to support an implementation of
EnumMessenger
and tensor variable elimination in NumPyro.I am implementing wrappers of these two functions in Pyro that construct
name_to_dim
/dim_to_name
automatically using the information inMarkovMessenger
, and I do not expect that Pyro users would ever construct these manually or even know that they exist.This design is very similar to the
dim_to_symbol
andsymbol_to_dim
arguments used inpyro.ops.packed.pack
andpyro.ops.packed.unpack
, but generalized to support event dimensions. As a consequence of this similarity, we should easily be able to replacepyro.ops.packed
with Funsor.Other changes
This design is also very similar to the conversion functions
funsor_to_tensor
andtensor_to_funsor
inpyro/ops/convert.py
, which use a particular convention for constructing instances ofdim_to_name
andname_to_dim
. I have modified the implementations of these functions to useto_data
andto_funsor
to reflect this similarity and to reuse their tests, and eventually I expect that much offunsor.pyro.convert
will be deprecated in favor of the plate/enum-aware wrappers being implemented in Pyro.I have also switched
to_funsor
to usefunctools.singledispatch
rather thanmultipledispatch
, which may provide a small performance boost and also avoids introducing amultipledispatch
dependency in the event that we move this code out of Funsor.Tested
test/pyro/test_convert.py
to_funsor
exercised by existing tests