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

Add planar trifocal tensor #1094

Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

Hal-Zhaodong-Yang
Copy link

This PR adds the following:

  1. Add TrifocalTensor2.[h|cpp], which declare and define planar trifocal tensor class TrifocalTensor2.
  2. Create TrifocalTensor2 constructors from bearing measurements and bearing measurements expressed in projective coordinates.
  3. Create class method transform to find a measurement in the first view using measurements from second and third views.
  4. Add unit test testTrifocalTensor2.cpp, which tests the constructed planar trifocal tensor.
  5. Test the transform method by comparing a set of ground-true bearing measurements with an estimated bearing measurements.

@@ -0,0 +1,128 @@
#include <iostream>
Copy link
Member

Choose a reason for hiding this comment

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

comment block missing

Matrix2 mat1() const { return matrix1_; }
};

} // namespace gtsam
Copy link
Member

Choose a reason for hiding this comment

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

no traits ???

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.

We need to thin k about the nonlinear case before reviewing

using namespace std;
using namespace gtsam;

TEST(TrifocalTensor2, transform) {
Copy link
Member

Choose a reason for hiding this comment

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

add a comment

}

// calculate trifocal tensor
TrifocalTensor2 T(measurement_u, measurement_v, measurement_w);
Copy link
Member

Choose a reason for hiding this comment

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

Change to a "named constructor", static FromBearingMeasurements()

Copy link
Member

Choose a reason for hiding this comment

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

Have a test for this method, and explain where the ground truth comes from!


// estimate measurement of a robot from the measurements of the other two
// robots
for (unsigned int i = 0; i < measurement_u.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

size_t

// robots
for (unsigned int i = 0; i < measurement_u.size(); i++) {
const Rot2 actual_measurement_u =
T.transform(measurement_v[i], measurement_w[i]);
Copy link
Member

Choose a reason for hiding this comment

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

comment that this is important and is being tested


// 2D landmarks
vector<Point2> landmarks;
landmarks.push_back(Point2(2.0, 0.5));
Copy link
Member

Choose a reason for hiding this comment

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

no copy/paste!


// 2D landmarks
vector<Point2> landmarks;
landmarks.push_back(Point2(2.0, 0.5));
Copy link
Member

Choose a reason for hiding this comment

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

copy/paste !!!!!??????


Matrix2 actual_trifocal_tensor_mat0 = T.mat1();

Matrix2 exp_trifocal_tensor_mat0;
Copy link
Member

Choose a reason for hiding this comment

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

Annotate this with //regression

@dellaert
Copy link
Member

dellaert commented Feb 9, 2022

Oh, and, BTW, CI seems to fail. Please do "make check" before doing a push to gtsam repo. As an aside, every push triggers CI, so please don't push every commit - push once you both are satisfied with changes. A development like this might be better done in a fork

@akshay-krishnan akshay-krishnan marked this pull request as draft February 26, 2022 02:02
@varunagrawal
Copy link
Collaborator

@Hal-Zhaodong-Yang do you intend to continue working on this?

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.

4 participants