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

Get Sim2 working #1090

Merged
merged 12 commits into from
Feb 7, 2022
Merged

Conversation

varunagrawal
Copy link
Collaborator

Fixed all the pending stuff in Sim2, plus also did some refactor of Sim3 to reflect comments in Sim2's original PR.

#include <gtsam/slam/KarcherMeanFactor-inl.h>

namespace gtsam {

using std::vector;

namespace {
namespace internal {
Copy link
Contributor

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)

Copy link
Collaborator Author

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.

Copy link
Member

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 :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup exactly :)

@varunagrawal
Copy link
Collaborator Author

@johnwlambert I merged in develop since some tests were failing because of that. It also seems that test_Sim2.py has a lot of tests failing because of the semantics not making complete sense. Can you look into that? It should be straightforward to run and test from this branch.

@varunagrawal
Copy link
Collaborator Author

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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

@johnwlambert johnwlambert left a comment

Choose a reason for hiding this comment

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

Really nice. LGTM.

@johnwlambert johnwlambert merged commit 10554d8 into add-Similarity2-classes Feb 7, 2022
@johnwlambert johnwlambert deleted the add-Similarity2-classes-2 branch February 7, 2022 17:26
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.

3 participants