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 more tests for collapse handler #809

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

fehiepsi
Copy link
Member

@fehiepsi fehiepsi commented Nov 7, 2020

Originally, this is used to test for funsor PR: pyro-ppl/funsor#392. But I found that we need more tests when updating the code to match Pyro version.

To pass some tests, I need to eagerly expand an ExpandedDistribution using _eager_expand_fn so that: Normal(0, 1).expand([10]) is still a Normal instance. This is not needed in Pyro because fn.expand(...) returns a new instance of type(fn).

I added some failing tests for discussions. Addressing them can be deferred to a follow-up PR in funsor.

@fehiepsi fehiepsi added the WIP label Nov 11, 2020
@fehiepsi fehiepsi added blocked and removed WIP labels Nov 12, 2020
@fehiepsi fehiepsi added WIP and removed blocked labels Nov 25, 2020
@fehiepsi fehiepsi changed the title Add test for beta plate binomial conjugacy Add more tests for collapse handler Nov 25, 2020
@fehiepsi fehiepsi requested review from fritzo and eb8680 November 26, 2020 04:45
fritzo
fritzo previously approved these changes Nov 30, 2020
@@ -245,6 +246,20 @@ def process_message(self, msg):
msg['stop'] = True


def _eager_expand_fn(fn):
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that this function eliminates lazy ExpandedDistributions and replaces them with eagerly tree_map-expanded distributions, and that this is needed because Funsor distributions do not support Expanded distributions? If so could you add a commend explaining that?

I guess an alternative would be to support ExpandedDistributions in Funsor, or to do this conversion in the funsor layer. @eb8680 does that seem possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. I'll add a comment for clarity. I thought from our last discussion, this stuff should be treated in Pyro/NumPyro?

Copy link
Member

Choose a reason for hiding this comment

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

I guess an alternative would be to support ExpandedDistributions in Funsor, or to do this conversion in the funsor layer.

We could certainly implement an equivalent of .expand. I think the easiest thing to do to address this case would be to add a to_funsor pattern for ExpandedDistribution.

Copy link
Member Author

Choose a reason for hiding this comment

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

add a to_funsor pattern for ExpandedDistribution

Yeah, it seems that you both agree to do this. I can take an effort for it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would basically be the same as what you have here, plus a final step that calls to_funsor on the eagerly expanded result.

@fehiepsi fehiepsi added this to the 0.5.1 milestone Jan 16, 2021
@fehiepsi fehiepsi modified the milestones: 0.5.1, 0.7 Mar 13, 2021
@fehiepsi fehiepsi modified the milestones: 0.7, 0.8 Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants