-
Notifications
You must be signed in to change notification settings - Fork 9
New Interface for Factor Graph Model Building #53
New Interface for Factor Graph Model Building #53
Conversation
* current version doesn't work on sanity_check notebook
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.
Haven't looked in detail, but maybe consider turning more things (wiring, evidence, etc.) into properties? You are already implementing getters/setters so might as well just use properties.....
* simplifies sanity_check example by using new interface
* function now takes variable group index instead of VariableGroup
* uses 'FactorFactories' to add factors to a FactorGraph instead of directly making the user pass these in * simplifies evidence construction for FactorGraphs * updates docstrings * updates sanity_check example to work with new interface
@StannisZhou : this update should contain everything we discussed relating to adding evidence and factors. Some things to look at in particular are the |
* fairly ugly hack in graph/update_evidence: if the tuple is size 1 extracts the 0th element...
@StannisZhou : added E2E unit test! I'm a little bit worried the results of the test are not consistent (so try running it yourself on your personal machine to see if it still works). The test is basically a scaled down version of the sanity_check example, so once the code is changed and the sanity check example runs again, it should be straightforward to run the test. |
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.
Made some comments regarding some details and the interface. But overall looks pretty good!
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.
Made some more comments. Mainly need to update the add_factors
interface.
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.
Last remaining piece: add support for adding individual factors (without the requirement that things have to be wrapped in a list).
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.
LGTM. Thanks for addressing the comments!
Initial implementation of new interface for model building!
(Currently, only evidence construction is new, but will add support for incremental factor adding and intuitive message initialization)
Resolves #52