-
Notifications
You must be signed in to change notification settings - Fork 37
Replace PrefixContext with model.prefix
#1011
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
base: breaking
Are you sure you want to change the base?
Conversation
Benchmark Report for Commit caa0c0fComputer InformationBenchmark Results |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1011 +/- ##
===========================================
Coverage ? 80.88%
===========================================
Files ? 40
Lines ? 3746
Branches ? 0
===========================================
Hits ? 3030
Misses ? 716
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #1011 is available at: |
180816d to
93ceec4
Compare
5278370 to
331279c
Compare
93ceec4 to
c670ef0
Compare
0bd4e02 to
8e3dcac
Compare
0b87d0d to
992569f
Compare
|
holy merge conflicts |
|
Agh, this is frustrating. |
9723158 to
bd7951c
Compare
bd7951c to
caa0c0f
Compare
What?
This PR removes
PrefixContext, and replaces it with an equivalent field on theModelstruct.Really?
I actually quite dislike this PR, and I don't think this is the correct direction to go in.
I think
PrefixContextwas easier to reason about because it allowed for a cleaner isolation of prefixing-related code. The absence of PrefixContext means that prefixes now get handled at different levels of the codebase, and is particularly messy when it comes to submodels.But, well, the work's done, so I may as well put it up.
More details?
There are two main 'types' of changes in this PR:
(1) Compiler
Inside the compiler, instead of just
vn(which is the lhs of tilde) we now have to handle bothleft_vn(which is the lhs of tilde) as well asvn(which is the result of applying any currently active prefixes).In effect, because the prefixing no longer happens in the tilde-pipeline, prefixes have to be handled at a higher level. For example, previously you could do something like:
Since
PrefixContextno longer exists, you have to prefix the VarName in the compiler itself before passing that to the general methodf(ctx, vn).This strategy works for functions like
getconditioned,hasconditioned, etc.(2) tilde-pipeline
For
tilde_assume!!, the above strategy is not enough. We also have to threadmodel.prefixthrough totilde_assume!!.The reason for this is quite subtle and has to do with submodels. Consider the following:
In this case, we want the sampled variable to be called
hello.a.x. If we use automatic prefixing as is shown here, this is easy to do: insidetilde_assume!!of the top-level model (1),awill already have been prefixed withhello, so theleftargument totilde_assume!!will behello.a. We can thus simply set this as the appropriate 'prefix' for the inner model.The problem arises if we don't automatically prefix:
If we run this, everything is the same as before: when we get to (1),
tilde_assume!!knows that the prefixed left-hand side ishello.a. However, because we don't want to prefix the submodel, the inner variable should only be calledhello.x, and nothello.a.x.This means that we have to store the prefix
helloseparately so that we can apply it to the submodel without theaas well.Note that in the absence of submodels, this would be entirely unnecessary.