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 calibrate with jacobians for Cal3Bundler #539

Merged
merged 9 commits into from
Sep 29, 2020

Conversation

varunagrawal
Copy link
Collaborator

Added an additional calibrate method for Cal3Bundler which compute jacobians.

Since the normal calibration method is iterative, I have approximated the jacobian as if a single iteration step has occurred, which should suffice assuming the projection is close to the ideal measurements.

I would definitely like a thorough review on this though.

Fixes #237

@varunagrawal varunagrawal added help wanted Need help and/or clarification bugfix Fixes an issue or bug labels Sep 23, 2020
@varunagrawal varunagrawal self-assigned this Sep 23, 2020
@varunagrawal
Copy link
Collaborator Author

Okay so this is breaking because calling code is getting confused which version of calibrate to call.

@dellaert should I merge the two calibrate functions into one?

@dellaert
Copy link
Member

Okay so this is breaking because calling code is getting confused which version of calibrate to call.

@dellaert should I merge the two calibrate functions into one?

Yes :-)

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.

It is not clear to me whether in the Jacobian section you should be using the initial estimate (x,y) or the final iterate. If the latter, then you should re-use the variables g, rr, etc.

gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
gtsam/gtsam.i Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
gtsam/geometry/Cal3Bundler.cpp Outdated Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator Author

varunagrawal commented Sep 29, 2020

@dellaert great call on the Implicit Function theorem. Thinking in math was the right approach 😄
This is now ready for review.

cc @johnwlambert

@varunagrawal varunagrawal force-pushed the fix/cal3bundler-jacobians branch from f2f98af to 8634d3f Compare September 29, 2020 05:50
CHECK(assert_equal(numerical2,K.D2d_intrinsic(p),1e-7));
Matrix Dcombined(2,5);
Dcombined << Dp, Dcal;
CHECK(assert_equal(Dcombined,K.D2d_intrinsic_calibration(p),1e-7));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These methods are deprecated, which is why I removed these tests.

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.

Awesome. You need to use fixed-size matrices if you know sizes, esp in functions like these that could be called for all points.

Maybe make an issue to hunt down and fix other implicit theorem instances?

// df/pi = -I (pn and pi are independent args)
// Dcal = -inv(H_uncal_pn) * df/pi = -inv(H_uncal_pn) * (-I) = inv(H_uncal_pn)
// Dp = -inv(H_uncal_pn) * df/K = -inv(H_uncal_pn) * H_uncal_K
Matrix H_uncal_K, H_uncal_pn;
Copy link
Member

Choose a reason for hiding this comment

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

These matrices should be fixed-size. Also, cache H_uncal_pn here? (In a fixed size Matrix) finally, you can avoid calculating H_uncal_K using `Dcal ? &H_uncal_K : nullptr in first argument below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quick question about caching H_uncal_pn: should I be calling uncalibrate with H_uncal_pn always?

@dellaert
Copy link
Member

Approved as I’ll trust you’ll make those final changes

@varunagrawal varunagrawal merged commit 1bbd233 into develop Sep 29, 2020
@varunagrawal varunagrawal deleted the fix/cal3bundler-jacobians branch September 29, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes an issue or bug help wanted Need help and/or clarification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch between calibrate functions for Cal3_S2 and Cal3Bundler
2 participants