From 7084a53c154783bced8c5e95eabdd3b74f727be8 Mon Sep 17 00:00:00 2001 From: gpolazzo Date: Mon, 20 Jul 2026 21:05:05 -0400 Subject: [PATCH] Fix Eigen::Ref Jacobian output args in MATLAB wrapper Eigen::Ref arguments are Jacobian output parameters in C++ but were previously treated as inputs in the MATLAB wrapper, making all Jacobian overloads permanently unreachable with the error: 'Arguments do not match any overload of function gtsam.Pose3.transformFrom' Root cause: Ref parses as a TemplatedType with typename.name 'Ref', not a plain Type with is_ref set. The wrapper emitted bogus isa(varargin{N},'Eigen.RefMatrixXd') checks that always return false, and tried to unwrap_shared_ptr from in[] on the C++ side. Fix: - Add is_eigen_ref() to CheckMixin to detect Ref TemplatedType - Exclude Ref args from varargin count and isa() checks in .m dispatch - Add nargout == N gate so Jacobian overload is selected when caller requests extra outputs - Allocate Eigen::MatrixXd locals in MEX instead of reading from in[] - Write Jacobian locals to out[1], out[2], ... after the primary return - Exclude Ref args from checkArguments count (nargin validation) Fixes borglab/gtsam#2492 --- gtwrap/matlab_wrapper/mixins.py | 16 +++++++ gtwrap/matlab_wrapper/wrapper.py | 38 +++++++++++++-- tests/fixtures/eigen_ref.i | 30 ++++++++++++ tests/test_matlab_wrapper.py | 79 ++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 tests/fixtures/eigen_ref.i diff --git a/gtwrap/matlab_wrapper/mixins.py b/gtwrap/matlab_wrapper/mixins.py index c59ab31..f2e63b8 100644 --- a/gtwrap/matlab_wrapper/mixins.py +++ b/gtwrap/matlab_wrapper/mixins.py @@ -22,6 +22,8 @@ class CheckMixin: ignore_namespace: Tuple = ('Matrix', 'Vector', 'Point2', 'Point3') # Matrix-like view types that can alias MATLAB double matrix storage. matrix_view_types: Tuple = ('ConstMatrixView', ) + # Eigen Ref types used as Jacobian output arguments (not inputs). + eigen_ref_types: Tuple = ('MatrixXd', ) # Methods that should be ignored ignore_methods: Tuple = ('pickle', ) # Methods that should not be wrapped directly @@ -74,6 +76,20 @@ def is_matrix_view(self, arg_type: parser.Type): """Check if `arg_type` should be unwrapped as a matrix view.""" return arg_type.typename.name in self.matrix_view_types + def is_eigen_ref(self, arg_type) -> bool: + """Check if `arg_type` is an Eigen Ref output argument (e.g. Ref). + + These are Jacobian output arguments in C++ that should be treated as + extra return values in MATLAB rather than input arguments. + Eigen::Ref parses as a TemplatedType with + typename.name == 'Ref' and a MatrixXd template parameter. + """ + return (arg_type.typename.name == 'Ref' + and hasattr(arg_type, 'template_params') + and len(arg_type.template_params) == 1 + and arg_type.template_params[0].typename.name + in self.eigen_ref_types) + def is_class_enum(self, arg_type: parser.Type, class_: parser.Class): """Check if arg_type is an enum in the class `class_`.""" if class_: diff --git a/gtwrap/matlab_wrapper/wrapper.py b/gtwrap/matlab_wrapper/wrapper.py index b7944b2..49927fc 100755 --- a/gtwrap/matlab_wrapper/wrapper.py +++ b/gtwrap/matlab_wrapper/wrapper.py @@ -301,11 +301,18 @@ def _wrap_method_check_statement(self, args: parser.ArgumentList): """ arg_id = 1 - param_count = len(args) + # Eigen Ref args are output arguments, not inputs — exclude from count. + eigen_ref_count = sum(1 for arg in args.list() + if self.is_eigen_ref(arg.ctype)) + param_count = len(args) - eigen_ref_count check_statement = 'if length(varargin) == {param_count}'.format( param_count=param_count) for _, arg in enumerate(args.list()): + # Eigen Ref args are outputs — skip isa() check entirely. + if self.is_eigen_ref(arg.ctype): + continue + name = arg.ctype.typename.name if name in self.not_check_type: @@ -340,6 +347,11 @@ def _wrap_method_check_statement(self, args: parser.ArgumentList): arg_id += 1 + # If there are Ref output args, require nargout to match. + if eigen_ref_count > 0: + check_statement += ' && nargout == {n}'.format( + n=eigen_ref_count + 1) + check_statement = check_statement \ if check_statement == '' \ else check_statement + '\n' @@ -361,6 +373,12 @@ def _unwrap_argument(self, arg, arg_id=0, instantiated_class=None): unwrap = 'unwrapMatrixView< {ctype} >(in[{id}]);'.format( ctype=arg_type, id=arg_id) + elif self.is_eigen_ref(arg.ctype): + # Ref is a Jacobian output arg — allocate locally, + # do not consume from in[]. Returned via out[] after the call. + arg_type = "Eigen::MatrixXd" + unwrap = 'Eigen::MatrixXd();' + elif self.is_ref(arg.ctype): # and not constructor: arg_type = "{ctype}&".format(ctype=ctype_sep) unwrap = '*unwrap_shared_ptr< {ctype} >(in[{id}], "ptr_{ctype_camel}");'.format( @@ -410,7 +428,9 @@ def _wrapper_unwrap_arguments(self, '''.format(arg_type=arg_type, name=arg.name, unwrap=unwrap)), prefix=' ') - arg_id += 1 + # Eigen Ref args don't consume an in[] slot — don't advance arg_id. + if not self.is_eigen_ref(arg.ctype): + arg_id += 1 params = '' explicit_arg_names = [arg.name for arg in args.list()] @@ -424,7 +444,8 @@ def _wrapper_unwrap_arguments(self, params += arg.default continue - if not self.is_ref(arg.ctype) and (self.is_shared_ptr(arg.ctype) or \ + if not self.is_eigen_ref(arg.ctype) and \ + not self.is_ref(arg.ctype) and (self.is_shared_ptr(arg.ctype) or \ self.is_ptr(arg.ctype) or self.can_be_pointer(arg.ctype)) and \ not self.is_enum(arg.ctype, instantiated_class) and \ arg.ctype.typename.name not in self.ignore_namespace: @@ -1366,6 +1387,9 @@ def wrap_collector_function_return(self, method, instantiated_class=None): params = self._wrapper_unwrap_arguments( method.args, arg_id=1, instantiated_class=instantiated_class)[0] + # Capture Ref output args before method may be reassigned to a string below. + eigen_ref_args = [arg for arg in method.args.backup.list() + if self.is_eigen_ref(arg.ctype)] return_1 = method.return_type.type1 return_count = self._return_count(method.return_type) @@ -1403,6 +1427,11 @@ def wrap_collector_function_return(self, method, instantiated_class=None): if return_count == 1: expanded += self._collector_return( obj, return_1, instantiated_class=instantiated_class) + + # Write any Eigen Ref (Jacobian) output args to out[1], out[2], ... + for i, ref_arg in enumerate(eigen_ref_args): + expanded += '\n out[{i}] = wrap< Eigen::MatrixXd >({name});'.format( + i=i + 1, name=ref_arg.name) elif return_count == 2: return_2 = method.return_type.type2 @@ -1570,7 +1599,8 @@ def generate_collector_function(self, func_id): min1='-1' if is_method else '', shared_obj=shared_obj, method_name=method_name, - num_args=len(extra.args.list()), + num_args=len([a for a in extra.args.list() + if not self.is_eigen_ref(a.ctype)]), body_args=body_args, return_body=return_body) diff --git a/tests/fixtures/eigen_ref.i b/tests/fixtures/eigen_ref.i new file mode 100644 index 0000000..e3dccd9 --- /dev/null +++ b/tests/fixtures/eigen_ref.i @@ -0,0 +1,30 @@ +namespace gtsam { + +#include + +class Pose3 { + Pose3(); + + // Case 1: two Ref args, primitive non-Ref input + gtsam::Point3 transformFrom(const gtsam::Point3& point) const; + gtsam::Point3 transformFrom(const gtsam::Point3& point, + Eigen::Ref Hself, + Eigen::Ref Hpoint) const; + + // Case 2: single Ref arg, no other inputs + gtsam::Pose3 inverse() const; + gtsam::Pose3 inverse(Eigen::Ref H) const; + + // Case 3: two Ref args, class-type non-Ref input + gtsam::Pose3 between(const gtsam::Pose3& pose) const; + gtsam::Pose3 between(const gtsam::Pose3& pose, + Eigen::Ref H1, + Eigen::Ref H2) const; + + // Case 4: static method with Ref arg + static gtsam::Pose3 Expmap(gtsam::Vector xi); + static gtsam::Pose3 Expmap(gtsam::Vector xi, + Eigen::Ref Hxi); +}; + +} \ No newline at end of file diff --git a/tests/test_matlab_wrapper.py b/tests/test_matlab_wrapper.py index 522955e..202cb3b 100644 --- a/tests/test_matlab_wrapper.py +++ b/tests/test_matlab_wrapper.py @@ -119,6 +119,85 @@ def test_matrix_view_arguments(self): self.assertIn('Eigen::Index m', header_content) self.assertIn('Stride(m, 1)', header_content) + def test_eigen_ref_jacobians(self): + """Test that Eigen::Ref args are treated as Jacobian outputs. + + Ref arguments should not appear as inputs in the MATLAB + dispatch check, and should be returned as extra output arguments + alongside the primary return value in the generated C++ MEX code. + Covers: primitive inputs, zero inputs, class-type inputs, static methods. + See https://github.com/borglab/gtsam/issues/2492 + """ + file = osp.join(self.INTERFACE_DIR, 'eigen_ref.i') + + wrapper = MatlabWrapper(module_name='eigen_ref', + top_module_namespace=['gtsam'], + ignore_classes=['']) + + wrapper.wrap([file], path=self.MATLAB_ACTUAL_DIR) + + cpp_file = osp.join(self.MATLAB_ACTUAL_DIR, 'eigen_ref_wrapper.cpp') + with open(cpp_file, 'r', encoding='UTF-8') as f: + cpp_content = f.read() + + m_file = osp.join(self.MATLAB_ACTUAL_DIR, '+gtsam', 'Pose3.m') + with open(m_file, 'r', encoding='UTF-8') as f: + matlab_content = f.read() + + # Must never emit the bogus Eigen.RefMatrixXd MATLAB class check. + self.assertNotIn('Eigen.RefMatrixXd', matlab_content) + # Must never try to unwrap Ref args from in[]. + self.assertNotIn('unwrap_shared_ptr< Eigen::MatrixXd >', cpp_content) + self.assertNotIn('unwrap< Eigen::MatrixXd >', cpp_content) + + # Case 1: transformFrom — primitive input (Point3) + 2 Ref args. + # MATLAB: only 1 varargin (point), nargout == 3. + self.assertIn( + "length(varargin) == 1 && isa(varargin{1},'double')" + " && size(varargin{1},1)==3 && size(varargin{1},2)==1" + " && nargout == 3", + matlab_content) + # C++: allocates both Ref args, passes without dereference, returns out[1]/out[2]. + self.assertIn('Eigen::MatrixXd Hself = Eigen::MatrixXd();', cpp_content) + self.assertIn('Eigen::MatrixXd Hpoint = Eigen::MatrixXd();', cpp_content) + self.assertIn('obj->transformFrom(point,Hself,Hpoint)', cpp_content) + self.assertIn('out[1] = wrap< Eigen::MatrixXd >(Hself);', cpp_content) + self.assertIn('out[2] = wrap< Eigen::MatrixXd >(Hpoint);', cpp_content) + self.assertIn('checkArguments("transformFrom",nargout,nargin-1,1);', cpp_content) + + # Case 2: inverse — zero real inputs + 1 Ref arg. + # MATLAB: length(varargin) == 0, nargout == 2. + self.assertIn('length(varargin) == 0 && nargout == 2', matlab_content) + # C++: single Ref arg allocated and returned via out[1]. + self.assertIn('Eigen::MatrixXd H = Eigen::MatrixXd();', cpp_content) + self.assertIn('obj->inverse(H)', cpp_content) + self.assertIn('out[1] = wrap< Eigen::MatrixXd >(H);', cpp_content) + self.assertIn('checkArguments("inverse",nargout,nargin-1,0);', cpp_content) + + # Case 3: between — class-type input (Pose3) + 2 Ref args. + # MATLAB: 1 varargin (pose object), nargout == 3. + self.assertIn( + "length(varargin) == 1 && isa(varargin{1},'gtsam.Pose3') && nargout == 3", + matlab_content) + # C++: pose unwrapped correctly, H1/H2 allocated and returned. + self.assertIn('Eigen::MatrixXd H1 = Eigen::MatrixXd();', cpp_content) + self.assertIn('Eigen::MatrixXd H2 = Eigen::MatrixXd();', cpp_content) + self.assertIn('obj->between(pose,H1,H2)', cpp_content) + self.assertIn('out[1] = wrap< Eigen::MatrixXd >(H1);', cpp_content) + self.assertIn('out[2] = wrap< Eigen::MatrixXd >(H2);', cpp_content) + + # Case 4: Expmap — static method + 1 Ref arg. + # MATLAB: 1 varargin (xi), nargout == 2. + self.assertIn( + "length(varargin) == 1 && isa(varargin{1},'double')" + " && size(varargin{1},2)==1 && nargout == 2", + matlab_content) + # C++: Hxi allocated and returned via out[1], nargin not decremented (static). + self.assertIn('Eigen::MatrixXd Hxi = Eigen::MatrixXd();', cpp_content) + self.assertIn('gtsam::Pose3::Expmap(xi,Hxi)', cpp_content) + self.assertIn('out[1] = wrap< Eigen::MatrixXd >(Hxi);', cpp_content) + self.assertIn('checkArguments("gtsam::Pose3.Expmap",nargout,nargin,1);', cpp_content) + def test_functions(self): """Test interface file with function info.""" file = osp.join(self.INTERFACE_DIR, 'functions.i')