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

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

Closed
dellaert opened this issue May 11, 2022 · 2 comments · May be fixed by #153
Closed

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

dellaert opened this issue May 11, 2022 · 2 comments · May be fixed by #153
Labels
bug Something isn't working

Comments

@dellaert
Copy link
Member

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
Copy link
Collaborator

@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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants