-
Notifications
You must be signed in to change notification settings - Fork 36
[Merged by Bors] - Support for submodels #233
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
Closed
Closed
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
55a00ce
added PrefixContext in preparation for allowing submodels
torfjelde 4f0ee95
added submodel macro
torfjelde 2625d12
forgot a QuoteNode and including the actual code
torfjelde 52c70a5
Update src/submodel_macro.jl
torfjelde 634a2ff
Update src/submodel_macro.jl
torfjelde 267a24e
added recursive constructor for PrefixContext to avoid runtime overhead
torfjelde ff539ce
warn is now false by default
torfjelde 2eb5eb4
removed unnecessary module-interpolation in submodel
torfjelde 902d527
use quot instead of QuoteNode to allow interpolation in submodel
torfjelde d05f080
change order of PrefixContext types
torfjelde 13bb259
change ordering type-parameters for PrefixContext
torfjelde 16d4210
escape the prefix to allow non-constant prefices
torfjelde 2c3ed99
use if generated for PrefixContext to allow the compiler to decide
torfjelde e0bb449
Update src/contexts.jl
torfjelde d47772c
_prefix_separator is now named PREFIX_SEPARATOR
torfjelde cdd2543
added tests for submodel macro
torfjelde c13418e
Merge branch 'master' into tor/submodels
torfjelde 46cdcb1
version bump
torfjelde 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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
macro submodel(expr) | ||
return quote | ||
_evaluate( | ||
$(esc(:__rng__)), | ||
$(esc(expr)), | ||
$(esc(:__varinfo__)), | ||
$(esc(:__sampler__)), | ||
$(esc(:__context__)) | ||
) | ||
end | ||
end | ||
torfjelde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
macro submodel(prefix, expr) | ||
return quote | ||
_evaluate( | ||
$(esc(:__rng__)), | ||
$(esc(expr)), | ||
$(esc(:__varinfo__)), | ||
$(esc(:__sampler__)), | ||
PrefixContext{$(esc(Meta.quot(prefix)))}($(esc(:__context__))) | ||
) | ||
end | ||
end |
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.
Uh oh!
There was an error while loading. Please reload this page.
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 guess, if the default value is
false
we can also just remove it since I doubt that anyone will enable the warnings explicitly. I am not completely sure if the warnings are useful anymore, in particular with the new variable names__varinfo__
etc. it seems unlikely thats someone would use the same name in their model definition. On the other hand, if we could ensure that official macros such as@addlogprob!
and@submodel
do not cause these warnings, I don't think there is any harm in keeping them.So if possible, I think it would be better to check in the macro expansion step of the compiler if it is one of the official macros and disable warnings for only the expression generated by them.
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.
@torfjelde What's your opinion?
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 weakly lean towards keeping this feature for developers.
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.
Oh sorry! But yes, I left it there because of the same reason as Hong said. I'm pro leaving it as is, and then if no one uses it for a long time, we might as well just drop it then. No need to rush completely removing it IMO.
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 thought one should not only keep it but also show warnings if not explicitly requested otherwise - i.e., I suggested reverting it back to
However, to avoid printing warnings if users use
@submodel
or@addlogprob!
I think one should disable warnings for the expanded code of these macros. It seems a simple if statement in the macro expansion inDynamicPPL.jl/src/compiler.jl
Line 193 in 7c8edab
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, I have to admit I don't like it either 😄 So I think I changed my mind and I would be fine with changing it to
warn=false
. Even though this changes the behaviour of@model
this won't break anyone's code. And in the next breaking release we might even consider removing thewarn
argument completely.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.
Haha, lovely 👍 True, plus I don't think I've ever come across anyone actually using these warnings...
I'll make default
false
and push 👍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.
Oh nvm, it's already this way, haha. I think this is good to go then!:)
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 know that it's used in DiffEqBayes since I added it there to avoid the warnings: https://github.com/SciML/DiffEqBayes.jl/blob/1749bc7ade1511d62a858eec4359705901126c92/src/turing_inference.jl#L53 😄 So as long as we do not suddenly remove it completely in a supposedly non-breaking release, it's fine.
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.
Haha, nice:) Good! I just merged with master and checking that tests run locally. Once that's done I'll bump version and it should be ready for bors!