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

Make it safer to use factor statements in guides #2952

Merged
merged 4 commits into from
Oct 27, 2021
Merged

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Oct 27, 2021

Blocking #2948

It has always been dangerous to use pyro.factor statements in guides #2829, because the guide doesn't know whether the factor arose from reparameterized or non-reparameterized sampling. This PR adds a has_rsample kwarg to pyro.factor and the underlying Unit distribution. The has_rsample need not be specified for pyro.factor statements in models, but is now required when using pyro.factor statements in guides. While this does cause some backwards incompatibility, I believe the likely small breakage will be worth introducing, so as to avoid silent errors (I've just spent half a week hunting down a bug in my own code due to this).

Tested

Copy link
Collaborator

@martinjankowiak martinjankowiak left a comment

Choose a reason for hiding this comment

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

lgtm.

"Defaults to False when used in models" how so?

"added tests of value in Fix AutoGaussian and tests #2948" can you pleae elaborate?

@fritzo
Copy link
Member Author

fritzo commented Oct 27, 2021

Thanks for reviewing!

"Defaults to False when used in models" how so?

By default dist.Unit.has_rsample = False, but in models this fact is not used in any inference algorithms.

"added tests of value in Fix AutoGaussian and tests #2948" can you pleae elaborate?

The strong high-precision tests of AutoGaussian in #2948 rely on the new behavior of pyro.factor(..., has_rsample=True). Since the new behavior is tested in that PR which is scheduled to merge soon, I have added only simple smoke tests in this PR.

@martinjankowiak
Copy link
Collaborator

re: The strong high-precision tests

can you please point to a specific test?

@fritzo
Copy link
Member Author

fritzo commented Oct 27, 2021

can you please point to a specific test?

@martinjankowiak martinjankowiak merged commit e2776a2 into dev Oct 27, 2021
@fritzo fritzo deleted the factor-in-guide branch February 24, 2022 14:19
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