-
Notifications
You must be signed in to change notification settings - Fork 267
Add more tests for collapse handler #809
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
Open
fehiepsi
wants to merge
15
commits into
pyro-ppl:master
Choose a base branch
from
fehiepsi:collapse_plate
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
cdef8d1
subclass Reparam
fehiepsi 614e81f
support collapse plate and test
fehiepsi 6d55b30
add some TODO to work on
fehiepsi 6d2e067
only expand if msg[fn] is a NumPyro Distribution
fehiepsi e1b4cd3
merge master
fehiepsi 210d2d5
Merge remote-tracking branch 'upstream/master' into collapse_plate
fehiepsi bab5329
merge master
fehiepsi 6612b9c
Merge remote-tracking branch 'upstream/master' into collapse_plate
fehiepsi 831db77
add various mvn tests
fehiepsi 8f8719f
address expanded distribution
fehiepsi cfdc30d
add more todo
fehiepsi ac09d78
make expand works under funsor
fehiepsi da3dd01
mark xfail for failing tests
fehiepsi c2f96ef
lint
fehiepsi f5b3b50
merge master
fehiepsi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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.
Do I understand correctly that this function eliminates lazy
ExpandedDistribution
s and replaces them with eagerlytree_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
ExpandedDistribution
s in Funsor, or to do this conversion in the funsor layer. @eb8680 does that seem possible?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.
Yes, it is. I'll add a comment for clarity. I thought from our last discussion, this stuff should be treated in Pyro/NumPyro?
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.
We could certainly implement an equivalent of
.expand
. I think the easiest thing to do to address this case would be to add ato_funsor
pattern forExpandedDistribution
.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.
Yeah, it seems that you both agree to do this. I can take an effort for it.
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.
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.