-
Notifications
You must be signed in to change notification settings - Fork 220
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
Prior
should use PriorContext
#2170
Conversation
model, | ||
VarInfo(), | ||
SamplingContext( | ||
rng, DynamicPPL.SampleFromPrior(), DynamicPPL.PriorContext() |
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.
Should PriorContext()
just be the default leaf context for SampleFromPrior
in DynamicPPL?
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 yes, but I don't think that's going to work atm because SampleFromPrior
is used in too many different contexts as just "do a first sample" or "sample but also compute logjoint" or "don't sample, just evaluate the model for these parameters because I've set the delete
flag to false
"...
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 see, makes sense. Maybe we should rethink the design of SampleFromPrior
/SampleUniform
but that's not for this PR 🙂
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.
Indeed! This is related to TuringLang/DynamicPPL.jl#573
Pull Request Test Coverage Report for Build 7829993171
💛 - Coveralls |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2170 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 22 22
Lines 1386 1385 -1
======================================
+ Misses 1386 1385 -1 ☔ View full report in Codecov by Sentry. |
The
Prior
sampler should use thePriorContext
when sampling, notDefaultContext
.This fixes #2169.