-
Notifications
You must be signed in to change notification settings - Fork 802
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 camera and Calibration Jacobians in Python #1171
Conversation
cam1 = gtsam.PinholeCameraCal3Bundler()
cam1.deserialize('22 serialization::archive 19 1 0\n0 0 0 0 0 1 0\n1 1 0\n2 9.99727427959442139e-01 6.30191620439291000e-03 2.24814359098672867e-02 5.97546668723225594e-03 -9.99876141548156738e-01 1.45585928112268448e-02 2.25703977048397064e-02 -1.44202867522835732e-02 -9.99641239643096924e-01 0 0 -5.81446531765312802e-02 -3.64078342172925590e-02 -5.63949743097517997e-01 1 0\n3 0 0 5.18692016601562500e+02 5.18692016601562500e+02 0.00000000000000000e+00 0.00000000000000000e+00 0.00000000000000000e+00 -1.14570140838623047e-01 -3.44798192381858826e-02 1.00000000000000008e-05\n')
# order is important because Eigen is column major!
Dpose = np.zeros((2,3), order='F')
Dpoint = np.zeros((2,3), order='F')
Dcal = np.zeros((2,6), order='F')
cam1.project(np.array([ 0.43350768, 0.0305741 , -1.93050155]), Dpose, Dpoint, Dcal) |
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.
That’s amazing! Add a test in python?
Should we merge this after adding a large number of overloads?
gtsam/geometry/geometry.i
Outdated
@@ -850,6 +851,7 @@ class PinholeCamera { | |||
static gtsam::Point2 Project(const gtsam::Point3& cameraPoint); | |||
pair<gtsam::Point2, bool> projectSafe(const gtsam::Point3& pw) const; | |||
gtsam::Point2 project(const gtsam::Point3& point); | |||
gtsam::Point2 project(const gtsam::Point3& point, Eigen::Ref<Eigen::MatrixXd> Dpose, Eigen::Ref<Eigen::MatrixXd> Dpoint, Eigen::Ref<Eigen::MatrixXd> Dcal); |
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.
Ok, that’s very cool!
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.
See comments. Regarding wrapping everything:
Could we just do all camera derivatives as promised by PR? That's not all derivatives in whole of GTSAM, but a good start/example. Just part of the camera derivatives is neither here nor there I think.
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.
Approving modulo some style comments.
As far as modernizing the test, should be easy, but I understand if you're strapped for time.
@@ -584,7 +584,13 @@ class Cal3_S2 { | |||
|
|||
// Action on Point2 | |||
gtsam::Point2 calibrate(const gtsam::Point2& p) const; | |||
gtsam::Point2 calibrate(const gtsam::Point2& p, |
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.
Cal3_S2 is not a camera, so thank you for the extra work :-) But please update PR comment (do that anyway, it's very terse) to say you also provide calibration derivatives.
PS, speaking about extra work: why not just do this in the base class? Is that because it is virtual and we can't handle?
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.
Most are done in the Base class, but some of them actually are different than the base class. I decided that I should not touch anything that works, so I just kept all the original and added overloads.
Dp = np.zeros((2, 2), order='F') | ||
camera.calibrate(img_point, Dcal, Dp) | ||
|
||
self.gtsamAssertEquals(Dcal, np.array( |
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.
numpy arrays should be tested with np.testing.assert...
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.
internally gtsamAssertEquals also deal with NP arrays
@@ -139,6 +139,17 @@ def test_jacobian(self): | |||
self.gtsamAssertEquals(z, np.zeros(2)) | |||
self.gtsamAssertEquals(H @ H.T, 4*np.eye(2)) | |||
|
|||
Dcal = np.zeros((2, 10), order='F') |
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.
Now that we have derivatives, we no longer have to jump through the linearize hoop above for the earlier test. Can you read and update 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.
I think they test for different derivatives though
Dcal = np.zeros((2, 3), order='F') | ||
cam1.project(np.array([1, 1, 1]), Dpose, Dpoint, Dcal) | ||
|
||
self.gtsamAssertEquals(Dpoint, np.array([[1, 0, -1], [0, 1, -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.
np.testing...
I'll merge first, don't really have time for another round... |
Okidoke. Could you announce on the Google group, though? This is pretty cool and I’d like you to get the credit :-) |
@ProfFan Thank you very much! I will create a PR soon with additional jacobians wrappings for python. |
Closes #1170