Fix scoped template bug #180

varunagrawal · 2025-11-28T20:41:24Z

Fixes #175

varunagrawal · 2025-11-29T16:40:22Z

I am going ahead and merging this so I can add it to the Matlab fix PR borglab/gtsam#2310 in GTSAM.

Copilot

Pull request overview

This PR extends the template instantiation machinery and wrappers to better support scoped templates and a new SmartProjectionRigFactor specialization, while cleaning up type handling and some build configuration.

Changes:

Refactors the template instantiator helpers and parser Typename/TemplatedType handling to better represent names, namespaces, and template arguments, and adjusts InstantiationHelper’s API and tests accordingly (including the scoped template handling fix).
Adds support for SmartProjectionRigFactor<gtsam::PinholeCamera<gtsam::Cal3_S2>> across interface fixtures, expected Python (pybind11) and MATLAB wrapper outputs, and the MATLAB wrapper tests.
Fixes minor issues and improves clarity in wrapper generation and build configuration (e.g., docstring extraction in PybindWrapper, __repr__ implementations, and CMakeLists.txt install path variable usage).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/test_template_instantiator.py	Updates imports and `InstantiationHelper` tests to match the refactored helper API and verifies scoped template and instantiation behavior.
tests/test_matlab_wrapper.py	Extends MATLAB wrapper tests to expect the new `SmartProjectionRigFactorPinholeCameraCal3_S2.m` file.
tests/fixtures/class.i	Adjusts interface fixture syntax/formatting and adds the `SmartProjectionRigFactor` templated class used for new instantiations.
tests/expected/python/class_pybind.cpp	Adds expected pybind11 bindings for `SmartProjectionRigFactor<gtsam::PinholeCamera<gtsam::Cal3_S2>>`.
tests/expected/matlab/class_wrapper.cpp	Adds expected MATLAB C++ bridge code for `SmartProjectionRigFactorPinholeCameraCal3_S2` and reindexes subsequent MyFactor wrappers.
tests/expected/matlab/SmartProjectionRigFactorPinholeCameraCal3_S2.m	Introduces the expected MATLAB wrapper class for `SmartProjectionRigFactorPinholeCameraCal3_S2`, including `add` overloads.
tests/expected/matlab/MyFactorPosePoint2.m	Updates expected MATLAB wrapper indices to align with the new function ID assignments in `class_wrapper.cpp`.
gtwrap/template_instantiator/helpers.py	Refactors instantiation helpers, especially `instantiate_type` for scoped templates and `InstantiationHelper`’s API and typing.
gtwrap/template_instantiator/function.py	Fixes `InstantiatedGlobalFunction.__repr__` to actually call the superclass `__repr__()` method.
gtwrap/template_instantiator/declaration.py	Simplifies fully-qualified name generation for instantiated forward declarations using the new `Typename` API.
gtwrap/template_instantiator/classes.py	Uses `InstantiationHelper` for constructors/methods and reworks parent-class instantiation and cpp typename construction with the new `Typename` shape.
gtwrap/pybind_wrapper.py	Tightens type hints (e.g., for `ArgumentList`) and restructures method-wrapping code for clarity, especially docstring injection and print-wrapper handling.
gtwrap/interface_parser/type.py	Redefines `Typename` construction/parse actions, adds helper methods (`get_template_args`, `templated_name`), and refactors `Type`/`TemplatedType` to use them.
gtwrap/interface_parser/function.py	Cleans up pyparsing imports for function parsing.
gtwrap/interface_parser/enum.py	Updates enum `cpp_typename` construction to use the new `Typename(name, namespaces)` API.
CMakeLists.txt	Fixes the `GTWRAP_CMAKE_INSTALL_DIR` variable to correctly expand `${INSTALL_CMAKE_DIR}`.

Comments suppressed due to low confidence (5)

gtwrap/interface_parser/type.py:196

This 'lambda' is just a simple wrapper around a callable object. Use that object directly.

    ).setParseAction(lambda t: Type.from_parse_result(t))

gtwrap/interface_parser/type.py:286

This 'lambda' is just a simple wrapper around a callable object. Use that object directly.

    ).setParseAction(lambda t: TemplatedType.from_parse_result(t))

gtwrap/pybind_wrapper.py:463

Variable module_var is not used.

            module_var = instance_name

gtwrap/interface_parser/type.py:344

This assignment to 'typename' is unnecessary as it is redefined before this value is used.

            typename = typename = "{typename}&".format(typename=typename)

gtwrap/interface_parser/type.py:15

Import of 'Union' is not used.

from typing import List, Sequence, Union

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T12:21:34Z

gtwrap/template_instantiator/helpers.py

+        new_typename = parser.type.Typename(
+            name=str_arg_typename.replace(scoped_template,
+                                          instantiation.templated_name()),
+            namespaces=instantiation.namespaces,
+        )


instantiate_type constructs a new Typename using parser.type.Typename, but gtwrap.interface_parser's public API does not expose a type submodule (it only re-exports symbols like Typename directly), so parser.type will raise an AttributeError at runtime. Use parser.Typename (or import Typename directly) instead of parser.type.Typename when creating new_typename.

Copilot · 2026-01-22T12:21:35Z

gtwrap/template_instantiator/classes.py

        if isinstance(self.original.parent_class, parser.type.TemplatedType):
-            return instantiate_type(
-                self.original.parent_class, typenames, self.instantiations,
-                parser.Typename(self.namespaces())).typename
+            namespaces = self.namespaces()
+            typename = parser.Typename(name=namespaces[-1],
+                                       namespaces=namespaces[:-1])
+            return instantiate_type(self.original.parent_class, typenames,


instantiate_parent_class checks isinstance(self.original.parent_class, parser.type.TemplatedType), but gtwrap.interface_parser does not define a type attribute, so this branch will never be taken and will likely raise an AttributeError when evaluated. To correctly detect templated parent classes, reference TemplatedType via the public API (e.g. parser.TemplatedType) or import TemplatedType from gtwrap.interface_parser.type explicitly.

Copilot · 2026-01-22T12:21:35Z

gtwrap/interface_parser/type.py

        """Get the TemplatedType from the parser results."""
-        return TemplatedType(t.typename, t.template_params, t.is_const,
-                             t.is_shared_ptr, t.is_ptr, t.is_ref)
+        return TemplatedType(t.typename, t.template_params.as_list(),


TemplatedType.from_parse_result calls t.template_params.as_list(), but pyparsing.ParseResults exposes asList() (capital "L") rather than as_list(), so this will raise an AttributeError during parsing of templated types. Convert the parse results using asList() or an equivalent (e.g. list(t.template_params)) so that the list of Type objects is built correctly.

Suggested change

return TemplatedType(t.typename, t.template_params.as_list(),

return TemplatedType(t.typename, list(t.template_params),

@varunagrawal I ran copilot review on your last PR here, and it did flag the issue. Gemini recommended to just use the deprecated call asList which is compatible across versions.

This is not correct. Using a deprecated function call is putting a bandage on a fracture and will come back to bite us.

Updating the requirements in the pyproject.toml is the way to go.

Agreed. Is that already updated?

It is.

wrap/pyproject.toml

Line 29 in 1740473

"pyparsing>=3.2.5",

I don't quite understand why Python/Pip is not respecting the requirement. Will look into it a bit more over the weekend after I finish the Hybrid State Estimation paper. :)

Copilot · 2026-01-22T12:21:35Z

tests/expected/matlab/SmartProjectionRigFactorPinholeCameraCal3_S2.m

+      obj = obj@gtsam.SmartProjectionFactorgtsam::PinholeCamera<gtsam::Cal3_S2>(uint64(5139824614673773682), base_ptr);
+      obj.ptr_SmartProjectionRigFactorPinholeCameraCal3_S2 = my_ptr;


The base-class constructor call obj = obj@gtsam.SmartProjectionFactorgtsam::PinholeCamera<gtsam::Cal3_S2>(...) appears malformed: it concatenates SmartProjectionFactor and gtsam::PinholeCamera<...> without a separator and mixes MATLAB dot notation with C++ :: syntax, which does not match the classdef base type on line 7 and is not a valid MATLAB class name. This should be updated to call the appropriate MATLAB base class (or handle superclass) using the same naming scheme as other inheritance wrappers (e.g. HessianFactor and ForwardKinematicsFactor).

Copilot · 2026-01-22T12:21:35Z

gtwrap/interface_parser/type.py

    rule = (
        namespaces_name_rule("namespaces_and_name")  #
-    ).setParseAction(lambda t: Typename(t))
+    ).setParseAction(lambda t: Typename.from_parse_result(t))


This 'lambda' is just a simple wrapper around a callable object. Use that object directly.

Suggested change

).setParseAction(lambda t: Typename.from_parse_result(t))

).setParseAction(Typename.from_parse_result)

Copilot · 2026-01-22T12:21:36Z

gtwrap/interface_parser/type.py

    ```
    """

    rule = (Or(BASIC_TYPES)("typename")).setParseAction(lambda t: BasicType(t))


This 'lambda' is just a simple wrapper around a callable object. Use that object directly.

Suggested change

rule = (Or(BASIC_TYPES)("typename")).setParseAction(lambda t: BasicType(t))

rule = (Or(BASIC_TYPES)("typename")).setParseAction(BasicType)

Copilot · 2026-01-22T12:21:36Z

gtwrap/interface_parser/type.py

    Here `gtsam::Matrix` is a custom type.
    """

    rule = (Typename.rule("typename")).setParseAction(lambda t: CustomType(t))


This 'lambda' is just a simple wrapper around a callable object. Use that object directly.

Suggested change

rule = (Typename.rule("typename")).setParseAction(lambda t: CustomType(t))

rule = (Typename.rule("typename")).setParseAction(CustomType)

varunagrawal added 12 commits November 28, 2025 13:46

fix scoped template bug

e688e47

rename union types to be singular

6e918ee

formatting

6ef1257

declare return type of instantiate_methods

3b66b89

rename ret to result

416c23c

refactor multilevel_instantiation to be more pythonic

1278e49

use f-strings in helpers.py

c4c98d5

refactor Typename constructor

f6dc175

improve Typename repr

53c6702

fix scoped template issue

eb94093

better repr to avoid regression

b1041a3

update tests

150c095

varunagrawal requested review from dellaert and p-zach November 28, 2025 20:41

varunagrawal self-assigned this Nov 28, 2025

fixes

2b5eaea

varunagrawal mentioned this pull request Nov 28, 2025

Fix Matlab wrapper borglab/gtsam#2310

Merged

varunagrawal merged commit 1740473 into master Nov 29, 2025
8 checks passed

varunagrawal deleted the fix-175 branch November 29, 2025 16:40

dellaert requested a review from Copilot January 22, 2026 12:14

Copilot started reviewing on behalf of dellaert January 22, 2026 12:14 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

	return TemplatedType(t.typename, t.template_params.as_list(),
	return TemplatedType(t.typename, list(t.template_params),

		obj = obj@gtsam.SmartProjectionFactorgtsam::PinholeCamera<gtsam::Cal3_S2>(uint64(5139824614673773682), base_ptr);
		obj.ptr_SmartProjectionRigFactorPinholeCameraCal3_S2 = my_ptr;

	).setParseAction(lambda t: Typename.from_parse_result(t))
	).setParseAction(Typename.from_parse_result)

	rule = (Or(BASIC_TYPES)("typename")).setParseAction(lambda t: BasicType(t))
	rule = (Or(BASIC_TYPES)("typename")).setParseAction(BasicType)

	rule = (Typename.rule("typename")).setParseAction(lambda t: CustomType(t))
	rule = (Typename.rule("typename")).setParseAction(CustomType)

Fix scoped template bug #180

Fix scoped template bug #180

Uh oh!

Conversation

varunagrawal commented Nov 28, 2025

Uh oh!

varunagrawal commented Nov 29, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

dellaert Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

varunagrawal Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

dellaert Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

varunagrawal Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants