Zero-output and Multi-output submodels

[ofSingularMind]

Feat. This PR enables specifying a @model with either multiple or zero factor/submodel arguments on the left-hand-side for use when that is advantageous or convenient.

[bvdmitri]

This is actually kinda cool @wouterwln but it definitely requires some clarification in the documentation. It might be a bit confusing for experienced Julia users. In Julia, there is a real convention (enforced by the compiler) that

function foo(x)
    a = x + 1
    b = x + 2
end

is strictly equivalent to

function foo(x)
    a = x + 1
    b = x + 1
    return b
end

so the last expression is always returned. Even if the return statement is missing. That contradicts the @model macro that not only returns the last expression, but returns everything that was in its input arguments. That is perfectly fine since everything inside @model macro is not "real" Julia and more like a domain specific language but only as long as it is documented clearly.

[wouterwln]

Apart from the compiler convention, I really like this idea. However, my question would be: In your setting, do you want to strictly enforce the ordering of missing arguments? So for example if I write (a, b) ~ two_output_sub(a = x), a would become b and b would become x? (intentionally confusing to make the point that this can get ugly). I'm fine with this if we strictly enforce the ordering, but we have to be careful. Fun fact: This would solve #269 as well imo (even though it's closed I don't think I actually fixed this??). Good work!

[ofSingularMind]

Does it make sense to require named tuples left and right for multi-output? So:

@model function my_sub_model(a, b, x)
   a ~ Normal(x, 1)
   b ~ Normal(a, 1)
end

@model function my_model()
   m_a ~ Normal(0, 1)
   m_b ~ Normal(0, 1)
   m_x ~ Normal(0, 1)
   (a = m_a, b = m_b) ~ my_sub_model(x = m_x)
end

Or we can support both (kwarg, posarg) with clear documentation that says, "For the sub_model arguments missing from the call, LHS variables provided without keyword arguments are interpreted positionally in order of the submodel definition, excluding the arguments provided to call on RHS."

[ofSingularMind]

I would have expected this model creation to fail because y has an open edge? Whereas x is provided data.

[bvdmitri]

GraphPPL doesn't care if the variable has an open edge or not, ReactiveMP does

[ofSingularMind]

Then probably ReactiveMP will need updates to accommodate these two new GraphPPL patterns? Not sure. I've been using the patterns and ReactiveMP doesn't seem to mind

[bvdmitri]

ReactiveMP handles this here

[bvdmitri]

I don't think we want to support this, this reads weird

[ofSingularMind]

I'll just state the use-case. I'm not married to it.

Consider that we often think of the graph in a temporal, or left-right fashion. That can lead to thinking of some factors' variables as being inputs, and others being outputs. At the start of a temporal chain, you can specify priors over state variables, x[1] ~ Normal(0,1), where x[1] is reading as the output from a no-input (or constant-input) factor Normal(0,1). This output variable, x[1], then becomes an input to the first time-step, and the chain moves on. At the end of the chain, you may want to specify e.g. a goal prior, ~ exp(2 * x[end]) and this factor can be read as having no outputs, and one variable input, x[end].

[ofSingularMind]

Sorry, I misunderstood. I think you mean the "fully_closed" submodel (no exposed variables on LHS or RHS).

I guess I think it is fine so long as z is closed (latent) in the submodel instead of hanging like it is now. But it does read weird, yeah.

[wouterwln]

I'm also not the biggest fan of this syntax, especially since it needs a separate make_node! implementation as well to make work (so it's not a byproduct of multi-output submodels)

[ofSingularMind]

Maybe if it is implemented, even if not highlighted in the documentation, and someone correctly specifies all variables RHS, then it does no harm? Maybe instead of x ~ sub_model(a,b) reading as x is distributed per, we can read it as Hey GraphPPL, add this shit to the graph, thanks. hehe

Is it a valid interpretation to say (re: open/closed), LHS = open-edge variables and RHS = fully-connected variables, and so in current submodel syntax, we (I think?) always try to define a submodel where only one edge remains open? But I guess that does not apply for RxInfer nodes where x ~ Normal(m, v) does not imply m and v to be closed (or constant).

Another use-case? In this case, open/closed is not enforced or implied but we see each variable at least twice, so we are OK anyways? Even with open/closed-enforcement this would be allowed and be seen to produce (in FFG terms) two closed edges connected by two equality nodes, or in bipartite, two more edges coming out of the variable nodes where a variable is seen twice in the model macro.

@model function my_model(y, z, m_a, m_b, m_c, m_d)
   ~ my_submodel_A(a=m_a, b=m_b, c=m_c, d=m_d)
   y .~ my_submodel_B(a=m_a, b=m_b)
   z .~ my_submodel_C(c=m_c, d=m_d)
end

Anyways, just thinking aloud :)

[bvdmitri]

GraphPPL doesn't care if the variable has an open edge or not, ReactiveMP does

[bvdmitri]

Typically you want to test what exception you throw or what kind of text is inside, e.g.

@test_throws "some text" some_function()

checks if some text is present somewhere inside the error message printed

[wouterwln]

Good work!! I'm not the biggest fan of the zero argument LHS. I agree with Dmitry that a documentation pass / freshen of the tests is required, but definitely a nice addition.

[wouterwln]

I'm also not the biggest fan of this syntax, especially since it needs a separate make_node! implementation as well to make work (so it's not a byproduct of multi-output submodels)

[wouterwln]

Apart from the compiler convention, I really like this idea. However, my question would be: In your setting, do you want to strictly enforce the ordering of missing arguments? So for example if I write (a, b) ~ two_output_sub(a = x), a would become b and b would become x? (intentionally confusing to make the point that this can get ugly). I'm fine with this if we strictly enforce the ordering, but we have to be careful. Fun fact: This would solve #269 as well imo (even though it's closed I don't think I actually fixed this??). Good work!