-
Notifications
You must be signed in to change notification settings - Fork 768
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
Get Sim2 working #1090
Get Sim2 working #1090
Conversation
#include <gtsam/slam/KarcherMeanFactor-inl.h> | ||
|
||
namespace gtsam { | ||
|
||
using std::vector; | ||
|
||
namespace { | ||
namespace internal { |
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 PR @varunagrawal. Appreciate you taking the time to help out here.
Was curious the rationale for the internal
namespace (why its needed and the naming)
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 followed Frank's suggestion from your PR.
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 am assuming @varunagrawal this is because you need Sim3 for your A-> B :-)
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.
Yup exactly :)
@johnwlambert I merged in develop since some tests were failing because of that. It also seems that |
Okay I resolved all the tests 🎉 |
world and egovehicle frame translated by 15 meters w.r.t. each other | ||
""" | ||
Rx90 = Rot2.fromDegrees(90) | ||
R180 = Rot2.fromDegrees(180) |
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.
@varunagrawal everything looks great. Was curious why you changed this value from 90 to 180, though. Both are valid, as I recall, for the purpose of the test.
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 translation values don't make sense otherwise. If we rotate by 90 degrees one of the two trajectories should change in the y axis.
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.
Really nice. LGTM.
Fixed all the pending stuff in Sim2, plus also did some refactor of Sim3 to reflect comments in Sim2's original PR.