Add Similarity(2) support in gtsam/geometry #918
Conversation
gtsam/geometry/Similarity2.cpp
Outdated
| return T; | ||
| } | ||
|
|
||
| Similarity2::operator Pose2() const { |
There was a problem hiding this comment.
Casting Sim2 to Pose2 is a bit of an unusual cast, but quite useful for some of my use cases (sometimes ground truth is provided in this form for some datasets).
gtsam/geometry/Similarity2.h
Outdated
|
|
||
| /// Convert to a rigid body pose (R, s*t) | ||
| /// TODO(frank): why is this here? Red flag! Definitely don't have it as a cast. | ||
| GTSAM_EXPORT operator Pose2() const; |
There was a problem hiding this comment.
Interesting that I have comments in this new PR ?? Must have forgotten. In any case, comment holds: remove this cast operator, as I don’t think there is a valid way to cast down, only cast from Pose2 to Sim2.
There was a problem hiding this comment.
This is actually copy-pasted from Similarity3.h. We should probably address that there.
gtsam/geometry/geometry.i
Outdated
| static gtsam::Similarity2 Align(const gtsam::Pose2Pairs& abPosePairs); | ||
|
|
||
| // Standard Interface | ||
| const Matrix matrix() const; |
|
@johnwlambert take a look at this again? CI fails, too. |
|
@johnwlambert I think we should start thinking about merging this PR? I think the only thing missing is the closestto? I don’t think it’s appropriate to do that with a constructor, as I mentioned. |
|
I'm going to leave reviewing to @dellaert (or maybe someone else from gtsfm) since I am a contributor to this PR and hence biased. :) |
varunagrawal
requested review from
ayushbaid
and removed request for
varunagrawal
There was a problem hiding this comment.
Sorry about the delay, let's try to land this. Note I am thinking we should prefer Matrix versions (and do computation there) for all Align constructors and add Rot2 and Rot3 versions, but that's for a different day.
Not yet approving as want to review after comments have been addressed (or rebuffed).
| @@ -113,6 +113,18 @@ list<Point2> circleCircleIntersection(Point2 c1, double r1, Point2 c2, | |||
| return circleCircleIntersection(c1, c2, fh); | |||
| } | |||
|
|
|||
| Point2Pair means(const std::vector<Point2Pair> &abPointPairs) { | |||
There was a problem hiding this comment.
For this PR and my latest PR, I'm starting to think we should "light-deprecate" PointPairs and do the computation in Eigen.
https://eigen.tuxfamily.org/dox/group__TutorialReductionsVisitorsBroadcasting.html
There was a problem hiding this comment.
Should we save this for another PR in favor of landing this one?
We can create an issue to track it.
|
I couldn't find any references for AdjointMap of Sim(2) and a C++ based unit test file is missing. :( I'll push a test file with a single constructor test and then merge. |
We previously had Similarity(3).
This adds a basic interface for Similarity(2) which is quite useful for my current work. Mostly parallels the implementation in Similarity3.h / .cpp, except this is a bare bones interface without wedge, adjointMap, expmap, or logmap for now.