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.

Sign up for GitHub

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

Jump to bottom

Add generic unscaled_sample method to Distribution #323

Merged
merged 26 commits into from
Mar 29, 2020
Merged

Add generic unscaled_sample method to Distribution #323

merged 26 commits into from
Mar 29, 2020

Conversation

eb8680
Copy link
Member

@eb8680 eb8680 commented Mar 25, 2020 •
edited
Loading

This PR adds a Distribution.unscaled_sample method for rewriting Distribution funsors to Delta funsors with correct Dice factors computed generically from Pyro's TorchDistribution.score_parts methods.

Questions for review:

  • Is there a better way to test gradients generically?
  • Can we make the tests less expensive and more reliable?

@eb8680 eb8680 added enhancement New feature or request Blocked Blocked by other issues WIP labels Mar 25, 2020
@eb8680 eb8680 changed the base branch from to-funsor-distributions-2 to master March 26, 2020 21:36
@eb8680 eb8680 requested a review from fritzo March 27, 2020 01:04
fritzo
fritzo reviewed Mar 27, 2020
View reviewed changes
Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Nice gradient tests!

funsor/distributions.py Outdated Show resolved Hide resolved
funsor/distributions.py Outdated Show resolved Hide resolved
funsor/distributions.py Outdated Show resolved Hide resolved
funsor/distributions.py Outdated Show resolved Hide resolved
test/test_distributions.py Outdated Show resolved Hide resolved
fritzo
fritzo approved these changes Mar 29, 2020
View reviewed changes
Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

LGTM after a trivial nit

funsor/distributions.py Outdated Show resolved Hide resolved
@eb8680 eb8680 merged commit 1b6cec0 into master Mar 29, 2020
@eb8680 eb8680 deleted the distribution-sample branch March 29, 2020 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fritzo fritzo fritzo approved these changes

Assignees
No one assigned
Labels
awaiting review enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eb8680 @fritzo