Mismatch between calibrate functions for Cal3_S2 and Cal3Bundler

[varunagrawal]

Description

PinholeCamera can accept both Cal3_S2 and Cal3Bundler as calibrations, but the calibrate function is defined differently for each, such that when camera.backproject(pn, depth) is called, Cal3Bundler errors out.

Steps to reproduce

This unit test when added to testPinholeCamera.cpp raises the error:

TEST( PinholeCamera, Cal3Bundler) {
  Cal3Bundler calibration;
  Pose3 wTc;
  PinholeCamera<Cal3Bundler> camera(wTc, calibration);
  Point2 p(50, 100);
  Point3 point = camera.backproject(p, 10);
}

Expected behavior

PinholeCamera (and primarily it's base class PinholeBase) should be able to call calibration().calibrate without any errors for all calibration types.

Environment

N/A

Additional information

[varunagrawal]

This issue is easy to fix once we figure out how to handle the tolerance argument in Cal3Bundler so that the method signature is consistent.

[johnwlambert]

@varunagrawal It looks like we can't access the camera parameter of SfmData in the wrapper until we resolve this (https://github.com/borglab/gtsam/blob/develop/gtsam/gtsam.i#L2865). Any idea for a path forward for resolving the tolerance argument issue?

[varunagrawal]

This is something @dellaert and I need to discuss at a design level so I can't comment on that sigh.

I've had a PR ready for months actually, but it breaks the API signature which is what is causing the delay.

[dellaert]

I’m sorry, what’s the issue?

[varunagrawal]

Cal3_S2 has calibrate defined as

Point2 calibrate(const Point2& p, OptionalJacobian<2,5> Dcal = boost::none,
                   OptionalJacobian<2,2> Dp = boost::none) const;

while Cal3Bundler has it defined as

Point2 calibrate(const Point2& pi, const double tol = 1e-5) const;

aka no Dcal and Dp, so this causes issues with wrapping and use cases in optimization.

[varunagrawal]

@dellaert @johnwlambert the original PR I made is here: #257

All we need to do is figure out the best API method signatures and code in the Jacobians and we're done. 🙂

[dellaert]

“Causes issues” ?

[varunagrawal]

Using CalBundler gives the no-matching function error for calibrate. Actually, I think I can solve this in 15 minutes later tonight.

[johnwlambert]

Thanks Varun. So Pybind11 will not allow two separate classes to have the same method with different signatures? That seems extremely restrictive. Or is it the case that these two classes are related in a particular hierarchy and that causes problems?

[varunagrawal]

This was definitely an issue with the old Python wrapper so I don't know if this will work with Pybind11 or not.
My guess is that it won't because PinholeCamera has a backproject method that needs a calibrate with Jacobians, so if Pybind11 performs static analysis (via the compiler, which it should), it will see that code path as non-existential which is good.

On the other hand, even if you're able to wrap the function, if you try to perform a backprojection, your code will just throw an unhelpful segfault.