ReactiveMP.jl icon indicating copy to clipboard operation
ReactiveMP.jl copied to clipboard

Add `getnodefn` function in rules

Open bvdmitri opened this issue 2 years ago • 4 comments

This PR adds a new feature which allows to request a reference to a node function from the @rule macro. In addition @call_rule and @call_marginalrule now attempt to create a node object when calling the specified rule.

bvdmitri avatar Sep 27 '22 14:09 bvdmitri

Codecov Report

Base: 76.82% // Head: 76.75% // Decreases project coverage by -0.07% :warning:

Coverage data is based on head (2381b93) compared to base (996726d). Patch coverage: 61.36% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
- Coverage   76.82%   76.75%   -0.08%     
==========================================
  Files         203      203              
  Lines        6711     6738      +27     
==========================================
+ Hits         5156     5172      +16     
- Misses       1555     1566      +11     
Impacted Files Coverage Δ
src/ReactiveMP.jl 66.66% <ø> (ø)
src/nodes/gamma_mixture.jl 4.00% <7.69%> (-0.31%) :arrow_down:
src/node.jl 87.12% <62.50%> (-0.45%) :arrow_down:
src/rule.jl 55.09% <90.00%> (+0.42%) :arrow_up:
src/nodes/normal_mixture.jl 90.09% <92.30%> (+1.62%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Sep 29 '22 18:09 codecov-commenter

@albertpod @nimrais @abpolym This PR is important for the upcoming delta node implementation as it changes the way we work with the node function. However, all rules would need a small update if this is merged, so I would probably prefer for all tests to be completed. But the issue is big enough and we must not release the delta branch before this one is merged.

The underlying issue is that the current approach with the DeltaFn{f} does not work if f depends on some global variable that are defined outside of its local scope, e.g.

y = 1.0

f = (x) -> x + y

This simply breaks Julia with undefined reference error. The solution is this PR which forces each rule in delta node to write:

f = getnodefn(:out) # for delta forward function
f = getnodefn(:in, k) # or for backward node function

# as a feature I also added
f = getprodfn() # returns a node function defined as a pdf for stochastic nodes

This should solve the undefined reference error, but these getnodefn are not implemented for the delta node in this PR and must be implemented in a separate PR (or extra commit in the delta node branch).

bvdmitri avatar Sep 29 '22 20:09 bvdmitri

I didn't get how you wanted to proceed with it.

Shouldn't you PR to delta branches, after which we adopt rules and tests?

albertpod avatar Sep 30 '22 09:09 albertpod

@albertpod Yeah, I'm not entirely sure. I was thinking that it might be easier to first merge this PR to the master branch and then sync both delta branches with the master. This PR does not break anything in the master, so it can be merged. But you should be aware of the fact that it is breaking for the delta nodes and syncing with the master branch after this PR should be performed carefully (I will help).

bvdmitri avatar Sep 30 '22 09:09 bvdmitri