-
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
Streamline internal de/conditioning interface #776
Conversation
Pull Request Test Coverage Report for Build 12700023444Details
💛 - Coveralls |
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 makes a ton of sense to me, I really like it.
Just some nitpicking, really.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #776 +/- ##
==========================================
+ Coverage 86.13% 86.26% +0.12%
==========================================
Files 36 36
Lines 4336 4332 -4
==========================================
+ Hits 3735 3737 +2
+ Misses 601 595 -6 ☔ View full report in Codecov by Sentry. |
7161599
to
2597193
Compare
Thanks Penny, no more issue from my end |
This PR:
in
src/contexts.jl
, removes the methodsAbstractPPL.condition(values, ...)
in favour of directly using theConditionContext()
constructor.This is mainly inspired by Aqua's complaints about how we are pirating
AbstractPPL.{de,}condition
(see Add Aqua tests #775). I've tested locally and this PR would fix all of thecondition
-related type piracies. But also semantically, I think thatcondition
is something you do to a model, not a context (and AbstractPPL's API sort of specifies that too). Since contexts are DynamicPPL-internal, I think it makes sense that we use an different, internal, function to manipulate that.likewise, renames
AbstractPPL.decondition(context, ...)
todecondition_context(context, ...)
. Suggestions for other names welcome!shifts the bulk of the 'figuring-out-arguments-and-coercing-to-the-right-format' stuff into a helper function, called
_make_conditioning_values
. This allows us to have a much more specific type for theConditionContext
struct itself, while retaining the current flexibility of the model conditioning syntax.adds tests for all of the above
Going beyond the name changes, there are two changes to functionality, which are both in the new
decondition_context
:decondition_context
now checks if there are no more conditioned values left, and if so, unwraps the layer ofConditionContext
. This shouldn't really affect anything, it just makes for cleaner context trees. For example:decondition_context
also makes sure to deepcopy the previous conditioned values, to avoid surprises due to mutation inBangBang.delete!!
: