Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor rule matching #188

Merged
merged 5 commits into from
Jan 19, 2022
Merged

Refactor rule matching #188

merged 5 commits into from
Jan 19, 2022

Conversation

ThijsvdLaar
Copy link
Collaborator

This PR refactors Message matching for update rule inference. For future rules (e.g. for the planned CVI implementation), the outbound message type might not always be clear upon message rule inference. That is, a CVI node might define Message{FactorNode} as outbound type, even though it really sends a Message{GaussianMeanVariance} upon execution.

The PR introduces << as a "parametric inheritance" operator, such that e.g. Message{Gaussian} << Message{FactorFunction}, but !(Message{FactorFunction} << Message{Gaussian}). The match() function is then defined as a two-way parametric inheritance, such that match(Ma, Mb) = (Ma << Mb) || (Mb << Ma).

This implementation broadens and clarifies match functionality without impacting existing rule matching.

@ThijsvdLaar ThijsvdLaar marked this pull request as ready for review December 7, 2021 14:38
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #188 (c6bd5e7) into master (fc11951) will decrease coverage by 0.08%.
The diff coverage is 81.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   86.59%   86.50%   -0.09%     
==========================================
  Files          95       95              
  Lines        4400     4395       -5     
==========================================
- Hits         3810     3802       -8     
- Misses        590      593       +3     
Impacted Files Coverage Δ
src/ForneyLab.jl 100.00% <ø> (ø)
src/engines/julia/update_rules/beta.jl 29.03% <33.33%> (ø)
src/probability_distribution.jl 75.34% <33.33%> (-4.37%) ⬇️
src/engines/julia/update_rules/sample_list.jl 100.00% <100.00%> (+10.00%) ⬆️
src/factor_nodes/clamp.jl 94.44% <100.00%> (ø)
src/helpers.jl 62.26% <100.00%> (+1.47%) ⬆️
src/message_passing.jl 84.00% <100.00%> (-0.13%) ⬇️
src/update_rules/beta.jl 100.00% <100.00%> (ø)
src/update_rules/equality.jl 98.09% <100.00%> (ø)
src/update_rules/nonlinear_extended.jl 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc11951...c6bd5e7. Read the comment docs.

@albertpod
Copy link
Collaborator

Looks good to me, however, I will leave the final judgment for merging this to @semihakbayrak.

Copy link
Contributor

@semihakbayrak semihakbayrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ThijsvdLaar for "Refactor rule matching" PR. I think it is a necessary extension and I am happy to see this functionality but to be honest I am not that knowledgable about inheritance in Julia and actually encountered several problems before. Nevertheless, as it passed unit tests, I think we can merge it.

@ThijsvdLaar ThijsvdLaar merged commit 208861e into master Jan 19, 2022
@ThijsvdLaar ThijsvdLaar deleted the match branch January 19, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants