ReactiveBayes / RxInfer.jl

Julia package for automated Bayesian inference on a factor graph with reactive message passing
MIT License
254 stars 24 forks source link

@constraints simply ignore incomplete factorization constraints without any warning #118

Closed bvdmitri closed 9 months ago

bvdmitri commented 1 year ago

The following code ignores the factorization constraints and does not give any warning

@constraints begin
    q(x)q(y)
    # should be q(x, y) = q(x)q(y) but no error or warning is shown
end

Found by @ThijsvdLaar

bvdmitri commented 10 months ago

ping @wouterwln , check that this bug is not present in the new version

wouterwln commented 10 months ago

@bvdmitri this throws an UndefVarError on q, is that enough?

bvdmitri commented 10 months ago

We can throw a user-friendly error or something when we encounter smth which is unexpected (basically when not a single @capture captured the problematic expression)?

wouterwln commented 9 months ago

Coming back to this, I don't know if we can safely check if something is not captured by a single pipeline, since we do a postwalk over expressions and not over lines. A lot of expressions will not be captured by any pipeline. Any suggestions?

bvdmitri commented 9 months ago

I think the issue here is that q(x)q(y) is being transformed even though it first must a part of the lhs_ = rhs_ expression and only then it can be transformed. q(x)q(y) by itself should not be transformed and should be just a function call to q, or at least we could throw an error if q(x)q(y) expression is being captured outside of the lhs_ = rhs_.

wouterwln commented 9 months ago

That is exactly what happens, I could design an additional pipeline that triggers when it encounters a call to q that is not wrapped in a lhs_ = rhs_ or lhs_ :: rhs_ statement, but that seems a bit unsafe to me, don't you think?

bvdmitri commented 9 months ago

Yeah, it is unsafe, because there can a be a function named q(). So, technically a raw statement q(x)q(y) is not wrong, given that there is a global q function, the thing is that it works even if there is no global q function, which should simply throw q is not defined error IMO.

I think you already know by yourself that less transformation we do in macros is better ;)

wouterwln commented 9 months ago

This is fixed in https://github.com/biaslab/GraphPPL.jl/pull/117. I throw an ErrorException for now. We can design a custom warning/error ad hoc