Allow usage of array literals on LHS of ~

[torfjelde]

Currently:

julia> using DynamicPPL

julia> @model function gdemo6()
           # `assume` and literal `observe`
           m ~ MvNormal(2, 1.0)
           [10.0, 10.0] ~ MvNormal(m, 0.5 * ones(2))
       end
ERROR: LoadError: "Malformed variable name [10.0, 10.0]!"
Stacktrace:
  [1] varname(expr::Expr)
    @ AbstractPPL ~/.julia/packages/AbstractPPL/CyEC9/src/varname.jl:258
  [2] generate_tilde(left::Expr, right::Expr)
    @ DynamicPPL ~/.julia/packages/DynamicPPL/emAbx/src/compiler.jl:258
  [3] generate_mainbody!(mod::Module, found::Vector{Symbol}, expr::Expr, warn::Bool)
  ...

This also means that using variables in this manner on the LHS isn't allowed, e.g. observe on something [x, x].

Therefore, we might as well just treat it as literals. This also make it possible to write [x, x] ~ ... where x is an argument to the model.

This PR allows this by simply falling back to the literal observe whenever we have Meta.isexpr(left, :vect).

Maybe also worth pointing out that there are docstrings lying about indicating that such a model would be valid:

DynamicPPL.jl/src/context_implementations.jl

Lines 377 to 384 in 9083299

	"""
	dot_tilde_observe(ctx, sampler, right, left, vi)
	Handle broadcasted observed constants, e.g., `[1.0] .~ MvNormal()`, accumulate the log
	probability, and return the observed value.
	Falls back to `dot_tilde(ctx, sampler, right, left, vi)`.
	"""

.

[devmotion]

It seems this fails if one writes

[x, y] ~ ...

where x and y are not arguments of the model and would have to be sampled? Could it be restricted to only literals? Or literals and observations?

[devmotion]

So maybe only default to being an observation statement if it is an array of literals and otherwise determine it in isassumption at runtime?

[torfjelde]

Yep, that will result in those variables being treated as literal observations. But unclear to me how we can allow this for variables 😕

[torfjelde]

So maybe only default to being an observation statement if it is an array of literals and otherwise determine it in isassumption at runtime?

isassumption will never be reached though because it's an invalid VarName. Plus to be general about it we'd have to recursively check isassumption for all arguments in the vect, since we can have nested expressions, etc.

[devmotion]

I guess it is OK to check it recursively? The VarName issue is a bit more annoying but as long as we can't distinguish between observations and assumptions, I think it is safer to only allow literals and error in the other case. Since it doesn't work currently, it doesn't break anything 🤷

[torfjelde]

I guess it is OK to check it recursively?

Think that's going to be quite complicated though since when to perform the checks is unclear. Do you check isassumption for every subexpression or just leaves, etc.

Or are you referring to checking whether it's a literal or not? E.g. just traversing the expression, ensuring that all args are literals? If so, that makes 👍

The VarName issue is a bit more annoying but as long as we can't distinguish between observations and assumptions, I think it is safer to only allow literals and error in the other case.

Yeah fair, I'm fine that 👍

Since it doesn't work currently, it doesn't break anything shrug

Well, that also holds for also allowing it 😅 But I agree, I'll add a isliteral that just traverse the expression, checking the leaves are all Number.

[torfjelde]

bors try

[devmotion]

This might be a bit general, e.g., also tuples of numbers or arrays of tuples of numbers or arrays of arrays of numbers etc are considered as literals now, in contrast to what the docstring mentions. But maybe this was your intention?

In any case, I guess it would be good to add some simple tests.

[torfjelde]

That was my intention yeah. These might still throw errors later when calling the model, e.g. mby there's no impl for assume for a tuple, but figured it was a good idea to not be very restrictive in the actual model-macro as we/people might add implementations for their distribution or w/e.

In any case, I guess it would be good to add some simple tests.

Yeah I'll add some 👍

[torfjelde]

Good now?

[bors]

try

Build failed:

• Turing.jl

[torfjelde]

Waiting for new release of Bijectors.jl to hit. Should I update the compat bounds for test @devmotion ?

[devmotion]

Yes, probably good to update the compatibilities for the tests if this fixes the test errors.

[devmotion]

An additional thought: is it necessary to update the compatibilities also in Project.toml?

[torfjelde]

An additional thought: is it necessary to update the compatibilities also in Project.toml?

I guess the annoying part is that if it doesn't update automatically, i.e. some package restricts Bijectors.jl to an older version, you're stuck untill the package updates even though nor you or the package every runs into this error.

My general "rule" for whether to bump patch-version in compat entries is if the bug is significant and silent. This is neither so to me it's whatever.

[devmotion]

I don't mind either, let's just keep them for now then, I guess.

[torfjelde]

bors r+

[bors]

Build failed:

• Turing.jl

[devmotion]

@KDr2 It seems Libtask 0.5.2 broke something: https://github.com/TuringLang/DynamicPPL.jl/runs/2783332077#step:6:1112

It fails due to https://github.com/TuringLang/Libtask.jl/blob/c66b8c90a8413d4532aecf36cc157767ec51d68d/src/tarray.jl#L81 (the key does not exist in the dictionary), which is caused by the call of maximum with a TArray in the model (https://github.com/TuringLang/Turing.jl/blob/c79dce028b3edd0be284bf28b13d83956606b786/test/stdlib/RandomMeasures.jl#L24 and https://github.com/TuringLang/Libtask.jl/blob/c66b8c90a8413d4532aecf36cc157767ec51d68d/src/tarray.jl#L270). I'm not sure why the key does not exist though. Maybe it's sufficient to use get! or get with some default value, e.g., based on the original task?

[torfjelde]

Should I just add a compat bound for Libtask to the tests for now? Or ignore the error since we're popping out a new breaking release and so it won't be possible to run the Turing-tests anyways?

[yebai]

Should I just add a compat bound for Libtask to the tests for now? Or ignore the error since we're popping out a new breaking release and so it won't be possible to run the Turing-tests anyways?

Sounds good to ignore the error for now - @KDr2 should be able to fix the Libtask issue soon.

[KDr2]

I'm not sure why the key does not exist though. Maybe it's sufficient to use get! or get with some default value, e.g., based on the original task?

Simply using get[!] with a default value may run us into an error in many scenarios, when current task is a copy of a copy of the original task, and the middle task (the first copy) has the data update before the the second copy.

I've checked the code, every time we copy a task (or more precisely a CTask), we assign a new value to that data dict with the newly copied task as its key in the function copy_tarrays. It there any chance the TArray is used in an ordinary Task but not a CTask in this case?

[KDr2]

It there any chance the TArray is used in an ordinary Task but not a CTask in this case?

But the older version didn't fail this testcase, right? Or it's a new testcase?

[devmotion]

No, it's an old test case that worked with Libtask 0.5.1. And everything works fine after upper bounding Libtask.

[KDr2]

I ran the testcase locally on master branch, it passed :) I will try again to reproduce the bug.

[yebai]

I ran the testcase locally on master branch, it passed :) I will try again to reproduce the bug.

Which version of Libtask are you using? I think @torfjelde capped the version of Libtask in Turing/DynamicPPL to in order to make CI pass.

[KDr2]

Ah, you are right, I reproduced the bug with the Libtask version loosed, I am on it now.