Rename Python reserved keywords in wrapper interface files

[jashshah999]

Summary

The generated .pyi stub files use lambda and global as parameter/method names, which are Python reserved keywords. This causes mypy to report syntax errors on the stubs.

Renames in .i wrapper files:

• lambda → lambda_ (parameter names in SmartProjectionFactor methods, LevenbergMarquardtOptimizer method name)
• global → global_ (parameter name in ReferenceFrameFactor)

The wrapper tool already handles method name remapping (generating self->lambda() calls under a lambda_ Python binding), so no C++ source changes are needed.

Fixes #2322

[dellaert]

Thanks !!!
2 requests:

• parameter names should be the same in Python and c++, for doc generation to work, so please c++ names as well
• In c++, xxx_ means instance variable, so maybe rename to _xxx

[jashshah999]

Thanks for the feedback!

For the parameter names (lambda → _lambda, global → _global), I can rename those in both the .i files and the C++ headers. However, lambda is used extensively as both a parameter name and local variable in SmartFactorBase.h (across createHessianFactor, updateAugmentedHessian, createRegularImplicitSchurFactor, createJacobianQFactor, createJacobianSVDFactor and their function bodies where it's passed to Cameras::PointCov(E, lambda, ...)). I'll do a full rename across those.

For the lambda() method on LevenbergMarquardtOptimizer -- renaming the C++ method itself would be a breaking API change. The wrapper already handles this by generating self->lambda() under a _lambda Python name. Should I still rename the C++ method, or keep it as-is with only the Python binding renamed?

Will push the updated changes shortly.

[dellaert]

Please limit renaming in c++ to parameter names. C++ bodies are not visible in Python anyway.

[jashshah999]

Updated! Changes:

• Switched to _xxx prefix (instead of xxx_) in both .i files and C++ headers
• Renamed lambda → _lambda in parameter names across SmartProjectionFactor.h, SmartStereoProjectionFactor.h, and their .i counterparts
• Renamed global → _global in ReferenceFrameFactor.h parameter name
• Renamed LevenbergMarquardtOptimizer::lambda() → _lambda() in .h, .cpp, and .i
• Updated body references within those functions to use the new parameter names
• Limited C++ changes to parameter names only (no local variable renames in function bodies)
• Updated expected wrapper test outputs (matlab + python)

[ProfFan]

This is not right, renaming should happen in the Python wrapper not C++ itself

[jashshah999]

@ProfFan Could you clarify what you mean? @dellaert's earlier comment asked to "please [rename] c++ names as well" so that "parameter names should be the same in Python and c++, for doc generation to work", and then followed up with "Please limit renaming in c++ to parameter names."

So the current changes rename only C++ parameter names (e.g. double lambda → double _lambda in function signatures) to match the .i files -- not local variables or function bodies.

The one exception is LevenbergMarquardtOptimizer::lambda() which is a method name, not a parameter. Are you saying I should revert that method rename and let the wrapper handle the mapping instead? Happy to do that.

[dellaert]

@ProfFan indeed, parameter names should match.
@jashshah999 we should not change the method name. Does that give problems in Python?

[jashshah999]

Nope, no problems! The wrapper generator handles the mapping -- it will expose lambda_ in Python while calling self->lambda() in C++. Reverted the method rename; only parameter names are changed now.

[jashshah999]

Gentle ping @dellaert @ProfFan -- I believe the latest revision addresses all feedback (reverted the method rename, kept only parameter renames for the Python reserved keywords). Would appreciate a final review when you get a chance. Thanks!

[ProfFan]

I don't think you need to change the names here, they should be handled in wrap only in the generated file:

    py::class_<gtsam::ManifoldEvaluationFactor<gtsam::Chebyshev1Basis, gtsam::Pose3>, gtsam::NoiseModelFactor, std::shared_ptr<gtsam::ManifoldEvaluationFactor<gtsam::Chebyshev1Basis, gtsam::Pose3>>>(m_, "ManifoldEvaluationFactorChebyshev1BasisPose3")
        .def(py::init<>())
        .def(py::init<gtsam::Key, const gtsam::Pose3&, const std::shared_ptr<gtsam::noiseModel::Base>, const size_t, double>(), py::arg("key"), py::arg("z"), py::arg("model"), py::arg("N"), py::arg("x"))
        .def(py::init<gtsam::Key, const gtsam::Pose3&, const std::shared_ptr<gtsam::noiseModel::Base>, const size_t, double, double, double>(), py::arg("key"), py::arg("z"), py::arg("model"), py::arg("N"), py::arg("x"), py::arg("a"), py::arg("b"));

[ProfFan]

basically, ONLY the .i files.

[dellaert]

basically, ONLY the .i files.

No, @ProfFan , this is incorrect. Since about a year, since we added better docs generation, argument names have to match in c++ and python. So if we change them in .i we should do this in c++ as well.

[ProfFan]

Thanks Frank, I didn't realize that. However, this issue is a common task in FFI, which I don't think should be handled by renaming C++ variables. Python has a lot of reserved words, are we going to avoid all of them?

[dellaert]

This was the approach taken by @p-zach - if there’s a better way, feel free to propose and implement :-)

[p-zach]

Yes, IIRC the metadata on the wrapped functions is minimal, and without same-named parameters it is very difficult/impossible to guarantee a match between a given C++ function and the Python function that wraps it - which would mean you couldn't be sure the documentation for a C++ function is being assigned to the correct Python function. This is mostly an issue with overloaded methods. I worked at it for a while and that was the best I could find - probably there is a better way to do it, but it might take refactoring of the wrap library itself.