-
Notifications
You must be signed in to change notification settings - Fork 15
294 rule marginalrule and average energy should check interface names #545
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
294 rule marginalrule and average energy should check interface names #545
Conversation
…tests.jl:rule
…s returns nothing (missing node?).
… check for @average_energy calls.
src/score/score.jl
Outdated
|
|
||
| q_names, q_types, q_init_block = rule_macro_parse_fn_args(inputs; specname = :marginals, prefix = :q_, proxy = :Marginal) | ||
|
|
||
| fform_type = Core.eval(__module__, fformtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not use eval, is very dangerous especially when macro generating the code (you have eval inside of eval). Is using the fuppertype (defined above) not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that the tests are failing exactly because of the eval, since it evaluates inside the ReactiveMP, but the structure in tests is defined outside of ReactiveMP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add a test, that actually checks that the error is being thrown by defining a rule with wrong interfaces
🤖 Code FormattingYour PR still has some code formatting issues. I've updated PR #559 with the necessary formatting changes. You can merge that PR into this branch to fix the code style check. Alternatively, you can run |
bvdmitri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some idea for this PR so lets not merge just yet
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
==========================================
+ Coverage 76.17% 79.58% +3.40%
==========================================
Files 207 211 +4
Lines 6149 6249 +100
==========================================
+ Hits 4684 4973 +289
+ Misses 1465 1276 -189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bvdmitri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @abpolym and @wouterwln for helping with this!
Relevant issue: #294
I would like to get a first review on the code.
The code now checks for misnamed interfaces, including extra interfaces etc (see issue).
Potential things to do:
basic test_rule macro invokationtest intest_rules.jlis breaking due to specification of a node that is unknown when the rule for that node is being called