-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add print method to Graph and HashGraph #70
Add print method to Graph and HashGraph #70
Conversation
<< " constraints:\n"; | ||
for (const auto& constraint : constraints_) | ||
{ | ||
stream << " - " << *constraint.second << "\n"; |
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.
Is there any way to pass some extra indentation to *constraint.second
? (same for *variable.second
below)
The main motivation to have proper indentation is that it looks like we could get YAML compliant outputs that could be use to visualize the graph with graphviz if we create a YAML to dot translator. I wonder if that was your plan?
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.
Is there any way to pass some extra indentation to *constraint.second? (same for *variable.second below)
Not currently. We could modify the print()
function to accept an indentation string. The problem with that being that there is no way to set that parameter when using the stream operator. We could drop the stream operator in favor of a toString()
method. Then you could:
std::cout << constraint.toString(" ") << std::endl;
we could get YAML compliant outputs
That was not something I had thought of. If the output looks like YAML, it is pure coincidence. Returning valid YAML is interesting. The one downside I see is formatting the matrices. I was planning on updating the covariance output to use Eigen's IOFormatter, to get pretty looking matrices. But it may be possible to create pretty valid YAML matrices and we both win.
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.
Regarding toString
, although it sounds like the only option, I'm afraid that returning an std::string
would be slower than using streams, which IIRC would manage things efficiently in terms of memory and IO.
Re. Eigen matrix, I think there should be an IOFormatter that should be YAML compliant. I could give that idea a try on another PR.
fuse_graphs/src/hash_graph.cpp
Outdated
for (const auto& variable : variables_) | ||
{ | ||
const auto is_on_hold = variables_on_hold_.find(variable.first) != variables_on_hold_.end(); | ||
stream << " - " << (is_on_hold ? "[H] " : "") << *variable.second << "\n"; |
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.
For variables on hold I'm writting [H]
, but I wonder if I could instead add another "field" like on_hold: true
(or false
). That way it'd be complaint with YAML, which seems to be the format you're trying to follow in all your outputs.
The downside though, is that it'd be harder to see if a variables it's on hold because the on_hold
field would be at the bottom of the variable output. But for automated tools it'd be perfectly fine.
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 added the "on hold" feature because Ceres supported it. However, it did not do what I thought it would do. It acts as though you placed a prior with complete certainty on the "on hold" variables. This results in the variable values being computed as you might expect, but the uncertainty on the "on hold" variables is zero. As such, I'm not sure what the utility of holding a variable is. So, I wouldn't worry overly about the "on hold" being hard to see. I'm not sure how often it will ever get used.
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 for the explanation. I've just changed this to the YAML compliant way: on_hold: true/false
.
{ | ||
stream << "HashGraph\n" | ||
<< " constraints:\n"; | ||
for (const auto& constraint : constraints_) |
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.
For now I simply print the constraints and variables, but I'm open to suggestions regarding what we print here. There's another map that has variables and constraints pairs that could be printed as well, either replacing what it's printed now, or maybe printing all (constraints list + variables list + variables to constraints list mapping).
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.
The variables and constraints are what I typically print as well. If anything, I'm wondering if this implementation should get moved to the Graph
base class. We can use getConstraints()
and getVariables()
to access the collections in a generic way.
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 think that could be the generic implementation in the base class, but I think we should still allow the user to override the default/base implementation, which I think even allows extending it (by calling the base class implementation explicitly from the child class implementation).
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.
This works for me as-is. If we want to move to a formatted toString()
in the future, we can do that in some future PR.
<< " constraints:\n"; | ||
for (const auto& constraint : constraints_) | ||
{ | ||
stream << " - " << *constraint.second << "\n"; |
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.
Is there any way to pass some extra indentation to *constraint.second? (same for *variable.second below)
Not currently. We could modify the print()
function to accept an indentation string. The problem with that being that there is no way to set that parameter when using the stream operator. We could drop the stream operator in favor of a toString()
method. Then you could:
std::cout << constraint.toString(" ") << std::endl;
we could get YAML compliant outputs
That was not something I had thought of. If the output looks like YAML, it is pure coincidence. Returning valid YAML is interesting. The one downside I see is formatting the matrices. I was planning on updating the covariance output to use Eigen's IOFormatter, to get pretty looking matrices. But it may be possible to create pretty valid YAML matrices and we both win.
fuse_graphs/src/hash_graph.cpp
Outdated
for (const auto& variable : variables_) | ||
{ | ||
const auto is_on_hold = variables_on_hold_.find(variable.first) != variables_on_hold_.end(); | ||
stream << " - " << (is_on_hold ? "[H] " : "") << *variable.second << "\n"; |
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 added the "on hold" feature because Ceres supported it. However, it did not do what I thought it would do. It acts as though you placed a prior with complete certainty on the "on hold" variables. This results in the variable values being computed as you might expect, but the uncertainty on the "on hold" variables is zero. As such, I'm not sure what the utility of holding a variable is. So, I wouldn't worry overly about the "on hold" being hard to see. I'm not sure how often it will ever get used.
{ | ||
stream << "HashGraph\n" | ||
<< " constraints:\n"; | ||
for (const auto& constraint : constraints_) |
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.
The variables and constraints are what I typically print as well. If anything, I'm wondering if this implementation should get moved to the Graph
base class. We can use getConstraints()
and getVariables()
to access the collections in a generic way.
Instead of "[H] ", we do "on_hold: true/false".
@svwilliams Are you waiting for any action from my side to merge this? 😃 |
He's waiting on my review. All of our PRs require two reviews. I'll try to get to it today. |
This allows to print the
HashGraph
, which is useful for debugging purposes.