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 a pyro.factor primitive #2022

Merged
merged 5 commits into from Aug 23, 2019

Conversation

@fritzo
Copy link
Collaborator

commented Aug 21, 2019

Resolves #1454

This implements a pyro.factor(name, log_factor) primitive, possibly the most-requested feature we've avoided adding. Previous workarounds include Bernoulli observations or Delta observations with a log_density argument (as suggested by @fehiepsi).

Design

This PR implements a more transparent cheaper alternative:

  • Native pyro.factor statements, and
  • a non-normalized dist.Unit distribution with empty data.

Alternatives include a Delta or Bernoulli observation, but both incur nontrivial overhead due to tensor ops.

Tested

  • added unit tests for Unit
  • added autoguide tests for pyro.factor
  • examples/rsa refactoring is covered by existing tests
  • pyro.contrib.gp refactoring is covered by existing tests
  • manually verified RSA examples and tutorials produce identical results before and after this PR

@fritzo fritzo added the enhancement label Aug 21, 2019

@fritzo fritzo requested a review from eb8680 Aug 21, 2019

@jpchen
Copy link
Member

left a comment

i'm quite fond of the Bernoulli/Delta hack 😛. i suppose you can replace the rsa example implementation too.

@eb8680
Copy link
Member

left a comment

Looks correct, do the RSA examples and tutorials produce the same results?

@fritzo

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

do the RSA examples and tutorials produce the same results?

Yes, the RSA examples and tutorials produce the same results (after repairing bitrot in the tutorials)

@jpchen

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

is the log_density kwarg in Delta still needed?

@fritzo

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

@jpchen yes the log_density kwarg in Delta still needed. You could have answered this by grepping.

@jpchen

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

ah woops i missed easy_guide 😅, i only looked in our examples tutorials and tests. there are a handful of pyro_models that need to be fixed after this PR lands as well.

@eb8680
eb8680 approved these changes Aug 23, 2019

@eb8680 eb8680 merged commit 86ea905 into dev Aug 23, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details

@eb8680 eb8680 deleted the factor-statement branch Aug 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.