-
Notifications
You must be signed in to change notification settings - Fork 13
Updates to Matlab wrapper #179
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the MATLAB wrapper to support custom templating within templated types (e.g., std::vector<T> where T is a template parameter) and improves interface file parsing robustness. The changes enable proper instantiation and wrapping of functions like FindKarcherMean(std::vector<T>& elements) with various template specializations.
Key changes:
- Fixed template parameter substitution for multi-templated functions (T1 -> actual type instead of generic T)
- Added support for templated types with vector arguments in function signatures
- Improved type name handling and formatting in the wrapper generation
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_matlab_wrapper.py | Added expected output files for new templated functions |
| tests/fixtures/functions.i | Fixed template parameter name (T->T1), added triangulatePoint3 and FindKarcherMean test cases |
| tests/expected/python/functions_pybind.cpp | Fixed type substitution in Python bindings (T->string/double) |
| tests/expected/matlab/*.m | Generated MATLAB wrapper functions for new templated functions |
| tests/expected/matlab/functions_wrapper.cpp | Generated C++ wrapper implementations with corrected type handling |
| gtwrap/template_instantiator/helpers.py | Whitespace formatting changes |
| gtwrap/template_instantiator/function.py | Refactored to_cpp() and updated repr method |
| gtwrap/matlab_wrapper/wrapper.py | Improved function name formatting for templated functions |
| gtwrap/matlab_wrapper/mixins.py | Added type name string conversion for dict key usage |
| gtwrap/interface_parser/type.py | Refactored get_typename() method and improved code organization |
| gtwrap/interface_parser/template.py | Updated documentation comment |
| gtwrap/interface_parser/function.py | Improved string formatting in repr methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dellaert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I had one question but if you think it correct let’s merge.
| @@ -0,0 +1,7 @@ | |||
| function varargout = FindKarcherMeanPoint2(varargin) | |||
| if length(varargin) == 1 && isa(varargin{1},'std.vectorgtsam::Point2') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is std.vectorgtsam really what’s intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, but this is a drawback of the Mex C API since there is no std::vector in C. We will have to migrate to the Mex C++ API (something we discussed in the past as a possible Google Summer of Code project).
This is an issue in other existing classes as well (such as Ordering).
This PR adds support to the Matlab wrapper for handling custom templating within templated types, e.g.
It also adds additional features to ensure the wrapper doesn't error out during interface file parsing.