borglab/wrap

Free function templating does not work (as expected)

Closed this issue · 2 comments

I think the global function templating is broken. It should not add the type to the function name (just create the overload for the function), and it also omits the namespace in the code, leading to a compile error

Instead of

    m_.def("triangulatePoint3Cal3_S2",[](
const gtsam::Pose3Vector& poses, boost::shared_ptr<gtsam::Cal3_S2> sharedCal, 
const gtsam::Point2Vector& measurements, double rank_tol, bool optimize, 
const gtsam::SharedNoiseModel& model){
return gtsam::triangulatePoint3<Cal3_S2>
(poses, sharedCal, measurements, rank_tol, optimize, model);},
py::arg("poses"), py::arg("sharedCal"), py::arg("measurements"), py::arg("rank_tol"), py::arg("optimize"), py::arg("model") = nullptr);

It should be

    m_.def("triangulatePoint3",[](
const gtsam::Pose3Vector& poses, boost::shared_ptr<gtsam::Cal3_S2> sharedCal, 
const gtsam::Point2Vector& measurements, double rank_tol, bool optimize, 
const gtsam::SharedNoiseModel& model){
return gtsam::triangulatePoint3<gtsam::Cal3_S2>
(poses, sharedCal, measurements, rank_tol, optimize, model);},
py::arg("poses"), py::arg("sharedCal"), py::arg("measurements"), py::arg("rank_tol"), py::arg("optimize"), py::arg("model") = nullptr);

@dellaert I was fixing some other things for my work and came across this.

The second point seems to have been fixed already. I ran a quick unit test and the namespace is being added correctly, so yay!
Regarding the first point, @gchenfc makes a good point in #153 where making this change is

  1. API breaking.
  2. Provides potential overload resolution issues.

I am more concerned about the second one, a simple case being size_t vs uint32.

I am closing this for now since the major bug has been resolved and the free function templating is a matter of design which would need a design review with @dellaert and other parties involved.