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

Refactor MakeATangentVector by using VectorValues and fix testcase failure #495

Merged
merged 4 commits into from
Aug 25, 2020

Conversation

jingwuOUO
Copy link
Contributor

Refactor MakeATangentVector by using VectorValues and fix testcase failure

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.

Great. But some details need to be cleared up before we can merge. Ask for a re-review when done?

gtsam/sfm/ShonanAveraging.cpp Outdated Show resolved Hide resolved
double minEigenValue = lambdas(0);
for (int i = 1; i < lambdas.size(); i++)
minEigenValue = min(lambdas(i), minEigenValue);
// const Matrix S = ShonanAveraging3::StiefelElementMatrix(Qstar3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re-enable commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncommented the code. But minEigenValue is never used, my suggestion is that maybe we can add a test like "EXPECT_DOUBLES_EQUAL(0, minEigenValue, 1e-11);" to make sure both the eigen value computed by Spectra and Eigen are in the same range?

gtsam/sfm/ShonanAveraging.cpp Outdated Show resolved Hide resolved
gtsam/sfm/ShonanAveraging.cpp Show resolved Hide resolved
gtsam/sfm/ShonanAveraging.cpp Outdated Show resolved Hide resolved
gtsam/sfm/tests/testShonanAveraging.cpp Outdated Show resolved Hide resolved
// Assumes key is 0-based integer
const auto v_i = v.segment<d>(d * i);
Vector xi = Vector::Zero(dimension);
double sign = pow(-1.0, round((p + 1) / 2) + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add

double sign0 = pow(-1.0, round((p + 1) / 2) + 1); 

before the for loop, and here initialize

double sign = sign0;

@jingwuOUO jingwuOUO changed the title Jingwu/shonan Refactor MakeATangentVector by using VectorValues and fix testcase failure Aug 25, 2020
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.

Perfect :-)

@jingwuOUO jingwuOUO merged commit 144db8e into develop Aug 25, 2020
@jingwuOUO jingwuOUO deleted the jingwu/shonan branch August 25, 2020 19:12
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.

2 participants