-
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
Refactor MakeATangentVector by using VectorValues and fix testcase failure #495
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.
Great. But some details need to be cleared up before we can merge. Ask for a re-review when done?
double minEigenValue = lambdas(0); | ||
for (int i = 1; i < lambdas.size(); i++) | ||
minEigenValue = min(lambdas(i), minEigenValue); | ||
// const Matrix S = ShonanAveraging3::StiefelElementMatrix(Qstar3); |
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.
Please re-enable commented out code
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.
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
// 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); |
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.
add
double sign0 = pow(-1.0, round((p + 1) / 2) + 1);
before the for loop, and here initialize
double sign = sign0;
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.
Perfect :-)
Refactor MakeATangentVector by using VectorValues and fix testcase failure