-
Notifications
You must be signed in to change notification settings - Fork 32
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
Move predict
from Turing
#716
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
1c1c907
move `predict` from Turing
sunxd3 bdf90b4
minor fixes
sunxd3 c7d08b0
Update test/ext/DynamicPPLMCMCChainsExt.jl
sunxd3 a425c41
fix test error by discard burn-in's
sunxd3 41471f6
add some comments
sunxd3 90d99ca
fix test error
sunxd3 ea23b7c
Update test/ext/DynamicPPLMCMCChainsExt.jl
sunxd3 76ef40f
refactor the code; add `predict` in Turing that takes array of varinfos
sunxd3 304b63e
Update model.jl
sunxd3 53b6749
Merge branch 'master' into sunxd/move_predict
sunxd3 fcd7c3d
stop using `PredictiveSample` type
sunxd3 3dc742a
Merge branch 'master' into sunxd/move_predict
sunxd3 30208ec
use NamedTuple
sunxd3 bf38627
remove predict with varinfos function
sunxd3 fd1277b
Merge branch 'master' into sunxd/move_predict
sunxd3 86eab6b
Merge remote-tracking branch 'origin/master' into sunxd/move_predict
penelopeysm 7b172e2
Merge branch 'master' into sunxd/move_predict
sunxd3 a3fc8b1
update implementation and tests; no longer using AdvancedHMC
sunxd3 da7fa1c
try fixing naming conflict
sunxd3 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 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 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 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 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 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 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 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 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
Oops, something went wrong.
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.
This is actually changing the behavior from Turing.jl's implementation. This will result in also including variables used in
:=
statements, which is not currently done.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.
not sure this is true,
https://github.com/TuringLang/Turing.jl/blob/c0a4ee936570d46cfcddc333a1f12404da75be24/src/mcmc/Inference.jl#L383
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.
Ooooh nice catch; thanks! Hmm, uncertain if this is desired behavior though 😕
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 saw your issue on
:=
, totally understand the concern here. But if we are not exportingpredict
, we can change this in near future, also we might want to usefix
in the future, so the behavior will be right then.We would need to make a minor release of
Turing
if we change this now.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.
But isn't this the purpose of this PR? To move the
predict
from Turing.jl to DynamicPPL.jl?Whether we're using
fix
or not is just an internal impl detail, and is not relevant for its usage, right?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.
Ideally, I would want this PR to do a proper implementation of
predict
in DynamicPPL. But now, I am okay with the PR being only a first step towards that.what I was trying to say is that, with
fix
it should have the right behavior (with regards to:=
). Of course not the only way to reach the desired behavior.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.
Improving it in a separate PR sounds good, but please create an issue to track @torfjelde's comment.