`condition` and `decondition` using traits from #286 without `ContextualModel` #294

torfjelde · 2021-07-28T18:12:54Z

No description provided.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…l into tor/conditioning

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…ionContext

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…l into tor/conditioning

…alues

src/context_implementations.jl

src/contexts.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

yebai

I'm halfway through reviewing this PR; will add more comments after dinner.

src/DynamicPPL.jl

yebai · 2021-08-02T17:41:44Z

src/DynamicPPL.jl

+    contextualize,
+    observations,
+    conditioned,


Do we actually want to export these functions?

Suggested change

contextualize,

observations,

conditioned,

It was intentional if that's what you mean, in particular I think people will want the observations / conditioned method. With that being said, maybe we shouldn't export in the next release but delay it until we've tried it out a bit and are happy with it?

yes, I think we want to apply the principle that we only export functions with a strong use-case. It would help to avoid frequent breaking changes and/or maintenance burdens since we are still experimenting with things.

Sounds good 👍

yebai · 2021-08-02T17:49:48Z

src/compiler.jl

+function contextual_isassumption(context::AbstractContext, vn)
+    return contextual_isassumption(NodeTrait(context), context, vn)
+end
+function contextual_isassumption(context::ConditionContext, vn)


The current implementation of contextual_isassumption looks slightly inconsistent because it dispatches using both contexts and traits. I think this is fine for now, but we probably want to consider improving it in future PRs.

I'm a bit uncertain what you mean here 😕 This pattern we're using all over the place. Traits represents a default implementation, but we we still allow direct overloads, e.g. here ConditionContext and PrefixContext needs special handling but the rest of the contexts can use the trait-implementations.

Notice how even the trait-impls above call the impl without the trait argument, thus allowing these specific overloads.

Not quite sure - mixing traits and types kind of makes the code hard to read. But I'm happy to leave this as a future effort, given that the functionality is correct.

yebai · 2021-08-02T18:10:28Z

src/compiler.jl

+                #    In this case, the below will return `true` since the first branch
+                #    will be hit.
+                # 3. We are working with a `ConditionContext` _and_ it's in the model arguments,
+                #    i.e. we're trying to override the value. This is currently NOT supported.


I'm confused by what will happen in the 3rd situation: will the current implementation error/warn, or it will fail/override silently?

Sorry, no this just means that in this scenario ConditionContext won't have any effect, i.e.

@model function demo(x) x ~ Normal() end m = demo(missing) (m | (x = 1.0))() # <= `x` is still considered `missing` because we can't change variables used in args through `condition`

Cool, let us add this to the docstring since it makes things clearer.

Will do 👍

I'll also add tests now that we've landed on taking this approach.

src/compiler.jl

Co-authored-by: Hong Ge <hg344@cam.ac.uk>

yebai

Excellent work, I left many comments below. Overall, I think the PR implemented condition/deconditioning in a nice non-breaking way.

I think the mixed-use of traits and nested contexts requires some further discussions, but happy to do that in a separate effort.

yebai · 2021-08-02T20:47:44Z

src/DynamicPPL.jl

+    contextualize,
+    observations,
+    conditioned,


yes, I think we want to apply the principle that we only export functions with a strong use-case. It would help to avoid frequent breaking changes and/or maintenance burdens since we are still experimenting with things.

yebai · 2021-08-02T20:48:19Z

src/compiler.jl

+                #    In this case, the below will return `true` since the first branch
+                #    will be hit.
+                # 3. We are working with a `ConditionContext` _and_ it's in the model arguments,
+                #    i.e. we're trying to override the value. This is currently NOT supported.


Cool, let us add this to the docstring since it makes things clearer.

yebai · 2021-08-02T20:50:24Z

src/compiler.jl

+function contextual_isassumption(context::AbstractContext, vn)
+    return contextual_isassumption(NodeTrait(context), context, vn)
+end
+function contextual_isassumption(context::ConditionContext, vn)


Not quite sure - mixing traits and types kind of makes the code hard to read. But I'm happy to leave this as a future effort, given that the functionality is correct.

yebai · 2021-08-02T20:55:12Z

src/context_implementations.jl

 require_particles(spl::Sampler) = false

-_getindex(x, inds::Tuple) = _getindex(view(x, first(inds)...), Base.tail(inds))
+_getindex(x, inds::Tuple) = _getindex(Base.maybeview(x, first(inds)...), Base.tail(inds))


Maybe add some doc on what are the possible x types to improve clarity.

All types are allowed with maybeview:) Also this is more properly addressed in #295

yebai · 2021-08-02T20:57:19Z

src/context_implementations.jl

 end
-function tilde_observe(context::MiniBatchContext, right, left, vname, vi)
-    return context.loglike_scalar * tilde_observe(context.context, right, left, vname, vi)
+function tilde_observe(context::MiniBatchContext, sampler, right, left, vi)


It seems that we are missing the argument vname here..

So this is actually because we never use tilde_observe with vname. See impl of tilde_observe!.

This is also why this passed through the tests; we have absolutely no tests for the vname-included versions (and some of contexts are missing impls for this even on master).

yebai · 2021-08-02T21:02:09Z

src/contexts.jl

+    values::NamedTuple{Names}, context::ConditionContext
+) where {Names}
+    # Note that this potentially overrides values from `context`, thus giving
+    # precedence to the outmost `ConditionContext`.


Can we at least throw a warning if overrides happen here? It might cause very subtle bugs otherwise.

Warning will slow things down + I don't think it should warn because what we're doing here is setting the conditioned value, right? And so if you're setting something, you expect that value to be the one that ends up in the model. This is the case with the above.

Also IMO, a seperate note, any such logic should go into condition and/or decondition, not ConditionContext constructor.

yebai · 2021-08-02T21:04:22Z

src/contexts.jl

+    getvalue(context, vn)
+
+Return the value of the parameter corresponding to `vn` from `context`.
+If `context` does not contain the value for `vn`, then `nothing` is returned,


If context does not contain the value for vn, then nothing is returned,

Do we need to distinguish from the case where the value of a variable is missing?

Nah, getvalue will return missing if missing is indeed the value, not otherwise (and nor should it).

IMO we should actually just error here rather than return nothing 👍

yebai · 2021-08-02T21:10:39Z

src/contexts.jl

+condition(; values...) = condition(DefaultContext(), (; values...))
+condition(values::NamedTuple) = condition(DefaultContext(), values)


Minor comment: maybe do not provide these convenient condition functions, but always requiring the user to pass a context instead. Semantically, it feels a bit odd to condition on a set of variables without a context/model.

I sort of view it as an alternative constructor of ConditionContext and I don't expect condition to be used externally on anything except with Model. But I agree that the semantics are a bit weird.

How about this? We make ConditionContext callable!

julia> @model function demo() x ~ Normal() return x end demo (generic function with 1 method) julia> m = demo(); julia> m() -0.5714991589000286 julia> cm = m |> condition(x=1.0); julia> cm() 1.0

This is actually pretty neat, because I can easily imagine a pattern where people have some specific conditioning they want to do on several instances of a model, e.g. different parameterizations, etc., in which case it makes sense to do something like

julia> conditioner = condition(x=1.0); julia> conditioner(m)() 1.0

yebai · 2021-08-02T21:14:19Z

src/contexts.jl

+    # is that the outermost `context` takes precendence, hence when resolving
+    # the `conditioned` variables we need to ensure that `context.values` takes
+    # precedence over decendants of `context`.
+    return merge(context.values, conditioned(childcontext(context)))


Let's throw a warning here when overrides happen.

Hmm, it's a bit strange to warn only when the user try to retrieve the conditioned values, no? If we're going to warn, I prefer you previous suggestion of doing so in condition instead. I'm still a bit on the edge regarding that though because it means that if you intentially want to override a value, you'll see warnings everytime you use the context + it will have a significant performance impact.

I.e. we really need a opt-out mechanism if we add it + I'm saying that condition is a setter and hence we shouldn't be surprised if a value is replaced.

src/model.jl

…n-model

torfjelde · 2021-08-04T04:09:01Z

src/contexts.jl

+"""
+    hasvalue(context, vn)
+
+Return `true` if `vn` is found in `context`.
+"""
+hasvalue(context, vn) = false
+
+function hasvalue(context::ConditionContext{vars}, vn::VarName{sym}) where {vars,sym}
+    return sym in vars
+end
+function hasvalue(
+    context::ConditionContext{vars}, vn::AbstractArray{<:VarName{sym}}
+) where {vars,sym}
+    return sym in vars
+end
+
+"""
+    getvalue(context, vn)
+
+Return value of `vn` in `context`.
+"""
+function getvalue(context::AbstractContext, vn)
+    return error("context $(context) does not contain value for $vn")
+end
+getvalue(context::ConditionContext, vn) = _getvalue(context.values, vn)
+
+"""
+    hasvalue_nested(context, vn)
+
+Return `true` if `vn` is found in `context` or any of its descendants.
+"""
+function hasvalue_nested(context::AbstractContext, vn)
+    return hasvalue_nested(NodeTrait(hasvalue_nested, context), context, vn)
+end
+hasvalue_nested(::IsLeaf, context, vn) = hasvalue(context, vn)
+function hasvalue_nested(::IsParent, context, vn)
+    return hasvalue(context, vn) || hasvalue_nested(childcontext(context), vn)
+end
+function hasvalue_nested(context::PrefixContext, vn)
+    return hasvalue_nested(childcontext(context), prefix(context, vn))
+end
+
+"""
+    getvalue_nested(context, vn)
+
+Return the value of the parameter corresponding to `vn` from `context` or its descendants.
+"""
+function getvalue_nested(context::AbstractContext, vn)
+    return getvalue_nested(NodeTrait(getvalue_nested, context), context, vn)
+end
+function getvalue_nested(::IsLeaf, context, vn)
+    return error("context $(context) does not contain value for $vn")
+end
+function getvalue_nested(context::PrefixContext, vn)
+    return getvalue_nested(childcontext(context), prefix(context, vn))
+end
+function getvalue_nested(::IsParent, context, vn)
+    return if hasvalue(context, vn)
+        getvalue(context, vn)
+    else
+        getvalue_nested(childcontext(context), vn)
+    end
+end


It should be possible to do away with the "nested" versions (though they might be useful in other case) if we make isassumption also return whatever value we're going to use. See my comment on isassumption for more info.

torfjelde · 2021-08-04T04:17:31Z

src/compiler.jl

+                if !($(DynamicPPL.inargnames)($vn, __model__)) ||
+                   $(DynamicPPL.inmissings)($vn, __model__)
+                    true
+                else
+                    $expr === missing
+                end
            else
-                # Evaluate the LHS
-                $(maybe_view(expr)) === missing
+                false


If we wanted to do away with the getvalue_nested and hasvalue_nested (again, these are convenient to have even if we're not using them here but obviously their utility is reduced significantly), we could make this return both the Bool corresponding to whether it's an assumption and maybe a value, i.e. nothing or the value we've extracted from context. It would end up looking something like

function isassumption(expr::Union{Symbol,Expr}) vn = gensym(:vn) val = gensym(:vn) return quote let $vn = $(varname(expr)), $val = $(DynamicPPL.contextual_isassumption)(__context__, $vn) if $val !== missing if !($(DynamicPPL.inargnames)($vn, __model__)) || $(DynamicPPL.inmissings)($vn, __model__) true else $expr === missing ? (true, missing) : (false, $val) end else false, missing end end end end

with contextual_isassumption now returning missing if it's considered an assumption or anything else if it's considered an observation.

Similarly we'd have to add an additional variable in generate_tilde, etc. which is used if we hit the observe branch and the value return from isassumption is not missing.

All in all, IMO it ends up looking more complicated than the current implementation of getvalue_nested, etc. so I'm not in favour of doing it but wanted to let people know it's an option.

Sounds good!

src/compiler.jl

yebai · 2021-08-04T17:29:38Z

bors try

bors · 2021-08-04T17:30:56Z

try

Build failed:

format

torfjelde · 2021-08-04T17:47:06Z

Format will fail here because of domluna/JuliaFormatter.jl#449 (comment)

torfjelde · 2021-08-04T17:51:33Z

Btw, I've left some responses to your comments.

Also, I need to add some tests for condition and decondition on models just to be sure that it interacts nicely with submodels, etc. This should already be covered by the tests for the context, but I just want to be 100% certain.

torfjelde · 2021-08-05T14:12:20Z

It seems like bors mistakenly closed this issue o.O I'll push the branch to remote again, and complete it.

This PR introduces `condition` and `decondition`. This is really just a reopening of #294 that I can't reopen directly due to the target branch now being deleted. Co-authored-by: Hong Ge <hg344@cam.ac.uk>

torfjelde and others added 30 commits July 15, 2021 20:08

added ConditionContext and ContextualModel

cd1c46d

Merge branch 'master' into tor/conditioning

b877656

removed redundant definition

b1106ee

return condition model by default

0f00771

formatting

c754291

forgot to include contextual model in previous commit

2d3f94c

fixed typos

3e5a79f

added some niceties to ConditionContext

d4e4238

added support for vectors of VarName

85a47eb

Update src/contexts.jl

c7dae8d

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'master' into tor/conditioning

14f5f57

Merge branch 'master' into tor/conditioning

d3b6485

upper-bound Distributions.jl in tests

75680b3

Merge branch 'master' into tor/conditioning

1692c03

Merge branch 'tor/conditioning' of github.com:TuringLang/DynamicPPL.j…

262c86b

…l into tor/conditioning

make the isassumption check using context extensible and nicer

f0ae744

Merge branch 'tor/upper-bound-distributions' into tor/conditioning

22fcae8

renamed type-parameter for ConditionContext

b990bb0

Update src/contexts.jl

8994cd7

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'master' into tor/conditioning

8da41c8

introduced convenient _getvalue method

94da453

overload tilde_assume rather than tilde_assume! and others for Condit…

4e74cf8

…ionContext

added contextual_isassumption for PrefixContext

f9cdfa9

implemented contextual_isassumption for all contexts

835a41e

improved the way ConditionContext works signficantly

5d110d5

forgot impl of dot_tilde_observe for ConditionContext

560ca83

Apply suggestions from code review

5635c3b

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

overload _evaluate rather than the model-call directly

e78dc65

Merge branch 'tor/conditioning' of github.com:TuringLang/DynamicPPL.j…

5f0e4a8

…l into tor/conditioning

Merge branch 'tor/context-traits' into tor/conditioning-with-traits

3a408cf

torfjelde added 4 commits August 1, 2021 07:55

had forgotten a return

b492041

added methods for extracting the conditioned/observed variables and v…

7ae9e3e

…alues

fixed a couple of tilde statements

3b31d35

added docstring to observations

cefb443

github-actions bot reviewed Aug 1, 2021

View reviewed changes

src/context_implementations.jl Outdated Show resolved Hide resolved

src/contexts.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

6086734

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

yebai reviewed Aug 2, 2021

View reviewed changes

Apply suggestions from code review

e579020

Co-authored-by: Hong Ge <hg344@cam.ac.uk>

yebai requested changes Aug 2, 2021

View reviewed changes

torfjelde added 5 commits August 4, 2021 02:27

Merge branch 'tor/context-traits' into tor/conditioning-with-traits-i…

2af625c

…n-model

dont export contextualize, observations and conditioned

1d3fa2b

added getvalue_nested and hasvalue_nested to be more explicit

da34dff

added a substantial amount of testing for ConditionContext

5e15b25

removed some debugging code

386e985

torfjelde commented Aug 4, 2021

View reviewed changes

src/compiler.jl Outdated Show resolved Hide resolved

torfjelde added 2 commits August 4, 2021 05:18

use maybe_view in isassumption check

52c9f04

added more extensive docstring for getvalue_nested and hasvalue_nested

65e7f71

yebai approved these changes Aug 4, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Aug 4, 2021

Try #294:

69a83f3

bors bot deleted the branch tor/context-traits August 5, 2021 12:32

bors bot closed this Aug 5, 2021

torfjelde mentioned this pull request Aug 12, 2021

[Merged by Bors] - condition and decondition #301

Closed

		condition(; values...) = condition(DefaultContext(), (; values...))
		condition(values::NamedTuple) = condition(DefaultContext(), values)

condition and decondition using traits from #286 without ContextualModel #294

condition and decondition using traits from #286 without ContextualModel #294

Uh oh!

Conversation

torfjelde commented Jul 28, 2021

Uh oh!

Uh oh!

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yebai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

`condition` and `decondition` using traits from #286 without `ContextualModel` #294

`condition` and `decondition` using traits from #286 without `ContextualModel` #294