Respect user-provided function signatures

[devmotion]

Currently, when a user defines a model

@model function test(x, y)
	x ~ Normal(0, 1)
	y ~ Normal(x, 1)
end

then we define both test(x, y) = ModelGen(....) and test(; x, y) = test(x, y). Since we do not define an arbitrary macro syntax but parse Julia functions, we should maybe stick more to the Julia syntax for functions and respect the user-provided function signatures, i.e., only define the function test(x, y) if the user only provides keyword arguments. If the user wants a function with keyword arguments, she would have to write

@model function test(; x, y)
	x ~ Normal(0, 1)
	y ~ Normal(x, 1)
end

instead. Users could still provide default arguments for both positional and keyword arguments but the behaviour would be the same as for a regular Julia function - keyword arguments you can specify in any order but positional arguments require a user to specify arguments in a specific order. Internally we would still keep a named tuple of all arguments (positional + keyword arguments), so the change would only be user-facing by not providing more definitions of test than what the user wrote down explicitly. IMO that would be less surprising for users since @model would not change anymore how the function signature is interpreted.

@mohamed82008 mentioned on Slack that this might cause problems with the string macros (logpdf"...." etc.). Can you elaborate what you had in mind? I assumed it would not affect any functionality but just require you to specify test(missing, missing) explicitly if x and y are not observed.

[mohamed82008]

The main issue is that I form a named tuple from the LHS and RHS of | in the logprob macro. These some times get passed to the model constructor as kwargs to do the right thing without worrying about the order of the arguments. For instance, it would be weird if only prob"x = 1, y = 1 | m = 1, s = 1, model = ..." worked but prob"y = 1, x = 1 | m = 1, s = 1, model = ..." didn't.

[devmotion]

I thought one could solve this issue by keeping a sorted tuple of positional arguments (i.e., (:x, :y) or as Val if needed for type inference) as well and then just pass the positional arguments as positional arguments and the rest as keyword arguments after parsing the string. But I wouldn't want to touch any internal logic with this suggestion at all, so it should really only affect parts where we actually have to call the constructor.

[mohamed82008]

Sounds like more work for less features 😛

[devmotion]

It's mostly driven by my desire for consistency I guess 😄

[devmotion]

Just had a look at prob_macro.jl, the macros don't have to be changed at all for this proposal. As the internals would be unchanged, the ModelGen structure would be still the same and the arguments would be saved in exactly the same way internally. Since the macros only operate with ModelGen neither the user-facing macro syntax nor the internal implementation would be affected by the suggestion in this PR.

To summarize: The suggestion in this PR is completely unrelated from all internal design choices and implementations of DynamicPPL, the only change would be to only define

test(x, y) = Model($evaluator, (x = x, y = y), $model_gen_constructor)

for the example above instead of

test(x, y) = Model($evaluator, (x = x, y = y), $model_gen_constructor)
test(; x, y) = test(x, y)

since the user only defined a function with two positional arguments. IMO the advantage would be that it is a less surprising behaviour and users would have full control over which function exactly is generated and hence could then define their own dispatch hierarchy if needed without DynamicPPL messing it up by providing additional methods.

[mohamed82008]

If you feel it's important, try to code it up and let's see if it's feasible to do this without losing any of the current features. I think it might be harder than you think to get this right. But I could be wrong :)

[devmotion]

Was fixed in #109.