borglab / wrap

Wrapper generator for C++ projects with multiple language support
Other
30 stars 10 forks source link

Free function templating does not work (as expected) #148

Closed dellaert closed 6 months ago

dellaert commented 2 years ago

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);
varunagrawal commented 8 months ago

@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.

varunagrawal commented 6 months ago

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.