Simplify `isassumption` check #271

torfjelde · 2021-07-08T01:02:18Z

This is useful for a couple of reasons:

Less expr-manipulation.
When working with large models it can often be useful to just use @addlogprob! and other fancy stuff to speed things up. In those cases one might want to have different code for whether one is evaluating the model's logjoint or sampling, e.g. if isassumption(...) then do something, otherwise do something else. Hence it's super-convenient if we have a isassumption that the user can call from within the model.

I also checked using #248 and the if-statement indeed compiles away, as expected.

EDIT: We could also add a @isassumption(x[i]) that includes the ismissing, which might be convenient.

torfjelde · 2021-07-08T01:24:40Z

src/utils.jl

+"""
+    @isassumption(x)
+
+Return `true` if `x` is an assumption and `false` otherwise.
+
+Should only be used within a model-definition.
+
+See also: [`isassumption`](@ref).
+"""
+macro isassumption(left)
+    return :(
+        $(DynamicPPL.isassumption)($(esc(:(__model__))), $(varname(left))) ||
+        ismissing($(esc(left)))
+    )
+end


Could also use this in generate_tilde instead of writing $(DynamicPPL.isassumption)(__model__, $vn) || ismissing($left) by hand.

I think this would be good. I would also be easier to modify the check.

devmotion · 2021-07-08T04:57:46Z

src/compiler.jl


 """
-    isassumption(expr)
+    isassumption(model::Model, vn::VarName[, value])


It seems we don't need the version that accepts a value? And we don't want it since otherwise we always have to evaluate the expression?

Correct, we don't need it, but I think it's useful for users that wanna go "is this considered an assumption or not?" and they can check using isassumption(model, @varname(x), x).

Wouldn't it be more useful for them if we define

macro isassumption(model, left) return isassumption_expr(model, left) end macro isassumption(left) return isassumption_expr(:__model__, left) end function isassumption_expr(model, left) return :( $(DynamicPPL.isassumption)($(esc(model)), $(esc(varname(left)))) || $(esc(left)) === $(missing) ) end

? Then users could just use @isassumption(model, x) instead of having to deal with @varname.

Maybe we shouldn't even add the version without model and just always provide __model__ explicitly.

So I actually did exactly this locally a couple of hours ago, but removed it because it won't work in the repl if left isn't defined, i.e. you can't do the equivalent of isassumption(model, vn) using the macro. This will IMO be more confusing to a user that just wants to check if a particular (model, vn) or (model, vn, value) combination is considered an assumption or not. I also don't think anyone will actually use @isassumption anywhere but within a @model, hence I figured we'd just leave it as is for now and then if someone asks the question we can just add it in.

Exactly, we could do isdefined, and I'm happy with that if you prefer 👍

Though I it should be

function isassumption_expr(model, left) return if isliteral(left) :(false) else :( $(DynamicPPL.isassumption)($(esc(model)), $(esc(varname(left)))) || ($(Expr(:escape, Expr(:isdefined, vsym(left)))) && $(esc(left)) === $(missing)) ) end end

because using @isdefined, it will be expanded in the scope of DynamicPPL:

julia> @isassumption model(4.0) x false julia> x = missing missing julia> @isassumption model(4.0) x false julia> @macroexpand @isassumption model(4.0) x :((DynamicPPL.isassumption)(model(4.0), (VarName){:x}()) || $(Expr(:isdefined, :(DynamicPPL.x))) && x === missing)

vs.

julia> @isassumption model(4.0) x true julia> @macroexpand @isassumption model(4.0) x :((DynamicPPL.isassumption)(model(4.0), (VarName){:x}()) || $(Expr(:isdefined, :x)) && x === missing)

However, the example also shows that the current logic for isassumption/@isassumption is not really correct. Independent of the macro discussions, I think @isassumption model(4.0) b should return false and @isassumption model(4.0) x should always return false (since in the model x already has the value 4.0).

Agreed. I also stumbled upon another issue: missings as specified in the Model type does not propagate to submodels 😕

I guess #268 would address what you're talking about, but there's still a question of how to support submodels.

But these issues should probably not be addressed in this PR; IMO this PR should just make it somewhat convenient to use the existing functionality of isassumption within a user-model.

Yes, this is definitely outside of the scope of this PR. #268 contains some interesting ideas but my feeling is that it is a quite drastic change and I am not completely convinced that every change is necessary or useful currently (e.g., intuitively I also thought that a lot could be done in a similar way with contexts). Maybe it would be better to change things more gradually, possibly also in a less breaking way.

I also just noticed that you dropped the check for inmissings, it seems? I think it would be nice to keep the API simple and only expose the macro version and drop the isassumption(model, vn, value) functions. On a second thought, I guess it would be helpful to have both a version with model argument (for checks in the REPL) and one without that uses __model__ (simplifies the use in the @model macro). Maybe we could just use

macro isassumption(left, model=:__model__) return isassumption(left, model) end function isassumption(left, model) varname = DynamicPPL.varname(left) vsym = DynamicPPL.vsym(left) return quote !inargnames($varname, $model) || inmissings($varname, $model) || (@isdefined($vsym) && $vsym === missing) end end

Then we (and users) could use @isassumption(left) in the model and it should be equivalent to what we use currently, and one could still check @isassumption left model in the REPL (with the known flaws, but it would still be useful if used properly).

my feeling is that it is a quite drastic change and I am not completely convinced that every change is necessary or useful currently

Agreed, but I've become more intrigued recently as there are some niceties that you get as a result, e.g. I don't think we can handle the failure case you noted above with b not in the model without an approach similar to that PR. But again, unclear if it's worth it given the major change it requires, and even if it achieves what we want (e.g. allow impl of decondition and condition). But I'll leave this discussion for that PR 👍

I also just noticed that you dropped the check for inmissings, it seems? I think it would be nice to keep the API simple and only expose the macro version and drop the isassumption(model, vn, value) functions. On a second thought, I guess it would be helpful to have both a version with model argument (for checks in the REPL) and one without that uses model (simplifies the use in the @model macro). Maybe we could just use

I didn't drop it, it's just hidden in the isassumption(model, vn), but I agree that it should just all be in the macro instead 👍

But I prefer @isassumption(x) and @isassumption(model, x) rather than @isassumption(x, model), i.e.

macro isassumption(left) return isassumption(:(__model__), left) end macro isassumption(model, left) return isassumption(model, left) end function isassumption(model, left) varname = DynamicPPL.varname(left) vsym = DynamicPPL.vsym(left) return quote !inargnames($varname, $model) || inmissings($varname, $model) || (@isdefined($vsym) && $vsym === missing) end end

I went with using isassumption in the model, which also allow passing symbol that evaluates to varname(left). This makes the expanded code somewhat less of a mouthful, in an attempt to alleviate the noise introduced by @isdefined (compared to old version).

It does have the drawback that @isassumption isn't being used anywhere in the code though, so maybe I should add a test?

EDIT: Eh, you know what, I'll just use the macro as it will make things easier to maintain.

src/compiler.jl

devmotion · 2021-07-08T04:59:22Z

src/compiler.jl

-    2. `x` is among the input data to the model but with a value `missing`, or
-    3. `x` is among the input data to the model with a value other than missing,
+    2. `x` is among the input data to the model but with `value === missing`, or
+    3. `x` is among the input data to the model with a `value !== missing`,


This is not true?

You're right 😕 I just matched the existing docs, but a bit confused as to why this was there.

Removed (3) now.

devmotion · 2021-07-08T04:59:44Z

src/compiler.jl

+isassumption(model::Model, vn::VarName, value::Missing) = true
+isassumption(model::Model, vn::VarName, value) = isassumption(model, vn)


Suggested change

isassumption(model::Model, vn::VarName, value::Missing) = true

isassumption(model::Model, vn::VarName, value) = isassumption(model, vn)

src/compiler.jl

src/utils.jl

devmotion · 2021-07-08T05:03:48Z

src/utils.jl

+
+Should only be used within a model-definition.
+
+See also: [`isassumption`](@ref).


Maybe we should explain more clearly what the differences are and when you should use isassumption and when @isassumption.

Improved the docstring; is current okay?

devmotion · 2021-07-08T05:05:00Z

src/utils.jl

+"""
+    @isassumption(x)
+
+Return `true` if `x` is an assumption and `false` otherwise.
+
+Should only be used within a model-definition.
+
+See also: [`isassumption`](@ref).
+"""
+macro isassumption(left)
+    return :(
+        $(DynamicPPL.isassumption)($(esc(:(__model__))), $(varname(left))) ||
+        ismissing($(esc(left)))
+    )
+end


I think this would be good. I would also be easier to modify the check.

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

torfjelde · 2021-07-08T12:53:24Z

Tests are passing btw.

src/compiler.jl

torfjelde · 2021-07-09T22:17:42Z

Btw @devmotion, I also added support for expressions such as x[:, i] which have been missing, e.g. the following works nicely now:

 julia> @model function demo2(x, y)
           m ~ Normal()
           x ~ Normal(m, 1.0)
           y[1] ~ Normal(x, 1.0)

           if length(y) > 1
               y[2:end] ~ MvNormal(length(y) - 1, 1.0)
           end

           return (; m, x, y)
       end
demo2 (generic function with 1 method)

julia> m = demo2(missing, [missing, 2.0, 3.0]);

julia> m()
(m = 0.4976316735942513, x = 0.44900997623088473, y = [0.2536455471649265, 2.0, 3.0])

julia> m = demo2(missing, [1.0, missing, missing]);

julia> m()
(m = -1.1722550192844885, x = -1.0166096659206363, y = Union{Missing, Float64}[1.0, 3.4953329133243147, -1.697987882528696])
julia> m = demo2(missing, [missing, missing, missing]);

julia> m()
(m = 1.065221683727756, x = 1.4561957002666124, y = Union{Missing, Float64}[1.327755997039602, 2.6958766955778954, -0.7770607374830496])

It of course isn't deducible at compile-time for those particular cases, but I think it's better to fall back to this rather than not having the feature + some ambiguous error message about a certain type not being supported in logpdf.

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

devmotion · 2021-07-10T23:25:03Z

src/compiler.jl

-function isassumption(expr::Union{Symbol,Expr})
-    vn = gensym(:vn)
+macro isassumption(left)
+    return esc(isassumption(:(__model__), left))


Could we use the other approach and only escape whatever has to be escaped? Usually this is safer and one does not require to interpolate DynamicPPL in the expressions.

I did that initially but kept finding cases where things would break so I just gave up 😅

I guess I'll try again.

I don't think we can avoid escaping the full expression if we want to use it within another macro 😕
E.g. addlogprob! and others also do this.

devmotion · 2021-07-10T23:26:53Z

src/compiler.jl

+
+    sym = vsym(left)
+    return :(
+        (!$(DynamicPPL.inargnames)($vn, $model) || $(DynamicPPL.inmissings)($vn, $model)) ||


See above, maybe we could avoid having to interpolate DynamicPPL if we only escape the relevant parts?

devmotion · 2021-07-10T23:29:46Z

src/compiler.jl

+is_entirely_missing(x) = false
+function is_entirely_missing(x::AbstractArray{>:Missing})
+    missings = missing .=== x
+    if all(missings)


Just check count(===(missing), x) to avoid the allocation and checking both all and any?

Good idea 👍

devmotion · 2021-07-10T23:32:18Z

src/compiler.jl

+julia> @macroexpand @isassumption(x)
+:((!((DynamicPPL.inargnames)((VarName){:x}(), __model__)) || (DynamicPPL.inmissings)((VarName){:x}(), __model__)) || $(Expr(:isdefined, :x)) && x === missing)
+
+julia> @macroexpand @isassumption m x


I guess it would be better to check the intended behaviour - the macro expansion is difficult to read and easy to break.

I was debating that myself, and happy to do what you suggest 👍

yebai · 2021-07-18T20:59:48Z

src/DynamicPPL.jl

    pointwise_loglikelihoods,
    # Convenience macros
    @addlogprob!,
+    @isassumption,


Do we need to export @isassumption? It seems it is only used internally by the compiler.

The motivation of this PR is to provide something that the end-user can use, e.g. if you really want to squeeze out performance you might want to do something like

if @isassumption(x) x ~ Dist() else @addlogprob! f(x) end

There currently is no good way of doing this, hence this PR.

I also think it's useful to expose a macro that users can use to check whether or not something is treated as an assumption, even when not used within @model. People are often confused by this 😕 But this is not the main-motivation behind the PR; the above is.

torfjelde · 2021-08-14T00:17:49Z

Closed in favour of #301 which introduces the contextual_isassumption which in turn covers most of the usecases.

torfjelde added 4 commits July 8, 2021 01:55

isassumption no longer acts on expressions

62a6524

simple unnecessary indexing fixed

3e84c8d

bumped version

1cd2ed1

added convenient macro

960035f

torfjelde commented Jul 8, 2021

View reviewed changes

torfjelde requested a review from devmotion July 8, 2021 01:24

devmotion reviewed Jul 8, 2021

View reviewed changes

torfjelde and others added 8 commits July 8, 2021 13:03

Update src/compiler.jl

ec2dfd3

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

use isassumption-macro in model instead of hardcoded check

faa0f49

use === instead of ismissing

938d4f9

removed incorrect statement in docs of isassumption

7c9d7eb

added more descriptive docs to isassumption macro

0da3e06

export isassumption macro

ba98fba

further improvements to isassumption macro docstring

fdd0a59

make example in isassumption a doctest

4783b6c

torfjelde mentioned this pull request Jul 8, 2021

Benchmarking #248

Merged

torfjelde added 5 commits July 9, 2021 00:37

implemented the isassumption macro properly

d56f950

improved isassumption further

e994391

use macro within model

a647f29

removed now-redundant code

d25b7ad

add support missing support for indexing using ranges and colons

a7dcb56

github-actions bot reviewed Jul 9, 2021

View reviewed changes

src/compiler.jl Outdated Show resolved Hide resolved

Update src/compiler.jl

edc9002

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

devmotion reviewed Jul 10, 2021

View reviewed changes

incorporated improvement suggested by @devmotion

b3bf0a3

yebai reviewed Jul 18, 2021

View reviewed changes

torfjelde added 2 commits July 19, 2021 05:18

Merge branch 'master' into tor/isassumption

b33494c

dont use macro within in macro, instead use the isassumption call

eb98a04

improved doctests for isassumption macro

f6e60ed

torfjelde closed this Aug 14, 2021

yebai deleted the tor/isassumption branch January 31, 2022 20:01

		isassumption(model::Model, vn::VarName, value::Missing) = true
		isassumption(model::Model, vn::VarName, value) = isassumption(model, vn)


		Should only be used within a model-definition.

		See also: [`isassumption`](@ref).

Simplify isassumption check #271

Simplify isassumption check #271

Uh oh!

Conversation

torfjelde commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

torfjelde Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

torfjelde Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

torfjelde commented Jul 8, 2021

Uh oh!

Uh oh!

torfjelde commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Simplify `isassumption` check #271

Simplify `isassumption` check #271

torfjelde commented Jul 8, 2021 •

edited

Loading

torfjelde Jul 8, 2021 •

edited

Loading

torfjelde Jul 8, 2021 •

edited

Loading

torfjelde Jul 9, 2021 •

edited

Loading

torfjelde Jul 9, 2021 •

edited

Loading

torfjelde commented Jul 9, 2021 •

edited

Loading

torfjelde Jul 19, 2021 •

edited

Loading