-
Notifications
You must be signed in to change notification settings - Fork 36
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
Draft
penelopeysm
wants to merge
20
commits into
py/remove-samplingcontext
Choose a base branch
from
py/no-prefix-context
base: py/remove-samplingcontext
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.
Draft
Conversation
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
Benchmark Report for Commit 93ceec4Computer Information
Benchmark Results
|
5 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## py/remove-samplingcontext #1011 +/- ##
=============================================================
- Coverage 81.46% 81.15% -0.32%
=============================================================
Files 39 40 +1
Lines 3825 3778 -47
=============================================================
- Hits 3116 3066 -50
- Misses 709 712 +3 ☔ 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
Note that, apart from being simpler code, Distributions.Uniform also doesn't allow the lower and upper bounds to be exactly equal (but we might like to keep that option open in DynamicPPL, e.g. if the user wants to initialise all values to the same value in linked space).
This should have been changed in #940, but slipped through as the file wasn't listed as one of the changed files.
5278370
to
331279c
Compare
93ceec4
to
c670ef0
Compare
0bd4e02
to
8e3dcac
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #1010. Maybe don't look at this until all the InitContext stuff is merged.
Most of this PR is boring stuff: moving code around, making sure things are prefixed when they should be, making sure things aren't prefixed when they shouldn't be.
There is one slightly awkward catch: we have to thread
model.prefix
through 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),a
will already have been prefixed withhello
, so theleft
argument 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
hello
separately so that we can apply it to the submodel without thea
as well.Note that in the absence of submodels, this would be entirely unnecessary.
It's quite possible that similar thing will happen with conditioning and fixed values: for 'normal' (i.e. non-submodel) models there's no need to pass it through, but specifically for submodels it is needed. Previously all this information would have been accessed via the
context
argument totilde_assume!!
so in a sense we are just making this more explicit.