-
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 test for subgraph preconditioner in shonan and refined error message #611
Conversation
@jingwuOUO can you please run |
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.
Small comment, but in general LGTM.
gtsam/linear/SubgraphBuilder.cpp
Outdated
@@ -383,7 +383,7 @@ Subgraph SubgraphBuilder::operator()(const GaussianFactorGraph &gfg) const { | |||
const vector<size_t> tree = buildTree(gfg, forward_ordering, weights); | |||
if (tree.size() != n - 1) { | |||
throw std::runtime_error( | |||
"SubgraphBuilder::operator() failure: tree.size() != n-1"); | |||
"SubgraphBuilder::operator() failure: tree.size() != n-1, might caused by disconnected graph"); |
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.
"Might be caused by disconnected graph"?
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.
Approved but please adopt Varun's suggestion on language
gtsam::LevenbergMarquardtParams::CeresDefaults(), "SUBGRAPH"); | ||
ShonanAveraging3::Measurements measurements; | ||
|
||
auto subgraphShonan = fromExampleName("toyExample.g2o", params); |
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.
could be helpful to add a small comment explaining what is inside the toyExample.g2o file? i.e. what is it's structure? Is this test designed to make sure a disconnected graph is caught, or instead to operate on just the largest tree in the forest if disconnected?
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.
Good advice! The test data is a fully connected graph. Currently Shonan in gtsam doesn't deal with disconnected graph, this is only a simple test case to check whether the subgraph option works. Once I added the part handling the disconnected graph, I will add more test cases.
Added test for subgraph preconditioner in shonan and refined the error message