Introducing addons for rules (+scale factors, +switch node, +mixture model) #215
Conversation
src/rule.jl
Outdated
| whereargs | ||
| ) do | ||
| return quote | ||
| $(on_index_init) | ||
| $(m_init_block...) | ||
| $(q_init_block...) | ||
| $(body) | ||
| _addons = () | ||
| _message = $(MacroHelpers.remove_returns(body)) |
There was a problem hiding this comment.
Be careful with removing return statements from the whole message function body, as this may break things if a rule contains something like (even without addons)
@rule MyNode(...) (args...) = begin
if condition
return 1
end
return 2
end
There was a problem hiding this comment.
Resolved by 68809b4. It counts the number of return statements per macro by performing some string matching. Although it is better to be replaced with a proper walk, but I didn't manage to get this working.
There was a problem hiding this comment.
Yeah, that is better, I wonder if its really necessary to use remove_returns for the body of the @rule macro, but rather only on the body of the @logscale macro. I would probably move the remove_returns statement from rule.jl:307 to rule.jl:417 inside macro logscale ... end.
| _message = $(MacroHelpers.remove_returns(body)) | |
| rule:jl:307 _message = body | |
| rule.jl:417 return MacroHelpers.remove_returns(output) |
I'm afraid that we did not really impose this restriction in the @rule macro in the past and it might be breaking. @logscale macro, however, is new and it may impose new restrictions on its body (such as no or only 1 return statement).
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…into rule_addons
src/marginal.jl
Outdated
|
|
||
| function Base.:(==)(left::Marginal, right::Marginal) | ||
| # We need this dummy method as Julia is not smart enough to | ||
| # do that automatically if `data` is mutable | ||
| return left.is_clamped == right.is_clamped && | ||
| left.is_initial == right.is_initial && | ||
| left.data == right.data | ||
| left.data == right.data && | ||
| left.addons == right.addons |
There was a problem hiding this comment.
We will need not to forget to add a == method for scale addon as well.
| vtag :: T | ||
| vconstraint :: C | ||
| msgs_names :: N | ||
| marginals_names :: M | ||
| meta :: A | ||
| addons :: X |
There was a problem hiding this comment.
Here (as I understand) are technically enabled_addons, but this is too long to type, not sure what would be a better field name
src/model.jl
Outdated
| pipeline :: P | ||
| addons :: A |
There was a problem hiding this comment.
Here is enabled_addons as well, but again I'm not sure if this is the better name
src/rule.jl
Outdated
| @@ -179,7 +180,7 @@ function call_rule_macro_parse_fn_args(inputs; specname, prefix, proxy) | |||
| output.args = map(v -> apply_proxy(v, proxy), any.args) | |||
| return output | |||
| end | |||
| return :($(proxy)($any, false, false)) | |||
| return :($(proxy)($any, false, false, ())) | |||
There was a problem hiding this comment.
That is an interesting one, we might think about how we actually are going to test rules with addons. As I understand this line of code technically does prevent you from testing such rules (for now).
src/rule.jl
Outdated
| error("Error in macro. Lambda body arguments specification is incorrect") | ||
|
|
||
| # check for number of return statements | ||
| @assert MacroHelpers.count_returns(body) < 2 "@rule macro contains multiple return statements" |
There was a problem hiding this comment.
I think I mentioned it somewhere else, but I think it would be better to move "remove return statements" routine inside the logscale macro such that the rule macro itself is unaffected.
src/rule.jl
Outdated
|
|
||
| possible_fix_definition = """ | ||
| @rule $(spec_fform)(:$spec_on, $spec_vconstraint) ($arguments_spec, $meta_spec) = begin | ||
| @rule $(spec_fform)(:$spec_on, $spec_vconstraint) ($arguments_spec, $meta_spec, $addons_spec) = begin |
There was a problem hiding this comment.
Lets talk about it, I'm not sure rule addons should be a part of the rule specification, they might add some extra functionality, but do we need different set of rules for different set of addons?
src/rules/gamma_mixture/switch.jl
Outdated
| @@ -5,7 +5,7 @@ | |||
| q_b::NTuple{N, GammaDistributionsFamily} | |||
| ) where {N} = begin | |||
| U = map(zip(q_a, q_b)) do (a, b) | |||
| return -score( | |||
There was a problem hiding this comment.
That is exactly why we should move the "remove return statements" out of the rule macro ;)
| @@ -37,7 +37,7 @@ end | |||
| """ | |||
| function constvar end | |||
|
|
|||
| constvar(name::Symbol, constval, collection_type::AbstractVariableCollectionType = VariableIndividual()) = ConstVariable(name, collection_type, constval, of(Message(constval, true, false)), 0) | |||
| constvar(name::Symbol, constval, collection_type::AbstractVariableCollectionType = VariableIndividual()) = ConstVariable(name, collection_type, constval, of(Message(constval, true, false, nothing)), 0) | |||
There was a problem hiding this comment.
Hm, I noticed that in other places you used an empty tuple to mock an empty list of addons, but here nothing has been used. We should probably think what is better (I'm ok with both as long as it is the same everywhere).
For solving #172