-
Notifications
You must be signed in to change notification settings - Fork 767
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
Added support for positioning in BayesNet plotting #1070
Conversation
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 with minor comments
|
||
for (auto conditional : *this) { | ||
for (auto conditional : boost::adaptors::reverse(*this)) { |
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.
Maybe a comment here why reversing the order? Like following a top-down order in printing?
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.
It is because Bayes nets are typically specified in reverse topological sort order, that's how they come out of elimination. Will add comments in the next PR to not restart CI for this one thing
std::map<Key, Vector2> variablePositions; | ||
|
||
/** | ||
* The position hints allow one to use symbol character and index to specify |
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 like this functionality
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.
Do we want to specify factor positions as well?
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.
Yeah, forgot about that. will add. Currently in graphVizFormatting, actually, but I need it here.
variablePositions
in DotWriterBayesNet
use them, if givenSymbolicBayesNet
andGaussianBayesNet
fromBayesNet
so they have this as wellThis supports graphs in the book that look like this: