Skip to content
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

Merged
merged 5 commits into from
Nov 30, 2020

Conversation

jingwuOUO
Copy link
Contributor

Added test for subgraph preconditioner in shonan and refined the error message

@varunagrawal
Copy link
Collaborator

@jingwuOUO can you please run git merge develop, and push again? You have an older version of the CI code which will cause CI to always fail.

Copy link
Collaborator

@varunagrawal varunagrawal left a 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.

@@ -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");
Copy link
Collaborator

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"?

Copy link
Member

@dellaert dellaert left a 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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@jingwuOUO jingwuOUO changed the title Jing/subgraph Added test for subgraph preconditioner in shonan and refined error message Nov 30, 2020
@jingwuOUO jingwuOUO merged commit 0b045fd into develop Nov 30, 2020
@dellaert dellaert deleted the jing/subgraph branch December 29, 2021 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants