Fix Matlab wrapper #2310
Merged
Fix Matlab wrapper #2310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60b680f71 Merge pull request #179 from borglab/templated-global-functions cb1fcdf6d fix class pybind test 3df0c2976 added missing tests 86447d1d3 fixes to support gtsam::Key as an alias f195602ec fix repr dcc2d1529 some minor refactor 227e0c6a7 more improvements 30d2465b9 use string version of name when formatting typename for use in dict 8534beb53 small improvements to matlab wrapper main code 055fee301 get_typename helper method 2a81d9260 added additional non-trivial functions to tests 509d9d963 better documentation fd5acb136 small cleanup of GlobalFunction class 82ab5e30a Merge pull request #178 from borglab/upgrade 4f169c792 Set minimum cmake version to be 3.22 to support Ubuntu 22.04 9f82c7ebf make minimum cmake version 3.19.2 since it supports Apple Silicon 74a4428f4 add additional authors bf6684f5b remove even more unneeded dependencies 36c266fa0 update lock file f64d94587 set min python version to 3.10 5ce2c0742 minor updates 7ede0563f improve CI thanks to uv 3eeb49748 use uv for project management 596f59235 use latest runner versions fe3609aca cmake version and CI python version upgrades git-subtree-dir: wrap git-subtree-split: 60b680f71a4c25cc7c1502df723d4227abb3c20c
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the Matlab wrapper that was broken by previous contributions. The changes span multiple areas including migration from setup.py to modern pyproject.toml with uv dependency management, updates to interface files, wrapper generation code improvements, and corrections to expected test outputs.
Key Changes:
- Migrated Python packaging from setup.py to pyproject.toml with uv.lock for dependency management
- Fixed template instantiation in the Matlab wrapper to properly handle templated functions and types
- Added support for
gtsam::Key(uint64_t alias) in Matlab wrapper - Corrected multiple interface file issues including namespace qualifications and return types
- Updated CI workflows to use uv instead of pip
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wrap/pyproject.toml | New modern Python packaging configuration replacing setup.py |
| wrap/uv.lock | Locked dependency file for reproducible builds |
| wrap/gtwrap/matlab_wrapper/wrapper.py | Fixed template function name generation and added Key type support |
| wrap/gtwrap/template_instantiator/function.py | Improved template instantiation logic with bug in repr |
| wrap/gtwrap/interface_parser/type.py | Added get_typename() method with redundant assignment bug |
| wrap/matlab.h | Added wrap/unwrap specializations for uint64_t (gtsam::Key) |
| wrap/tests/fixtures/functions.i | Fixed template parameter names and added new test functions |
| wrap/CMakeLists.txt | Updated minimum CMake version with variable reference bug |
| gtsam/**/*.i | Fixed namespace qualifications and return types across multiple interface files |
dellaert
reviewed
Nov 28, 2025
Member
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
I would like Hat and Vee back if possible. Also, copilot seems to have found some legitimate issues still? Finally, can you confirm uv requirement will not ripple through to GTSAM ?
Contributor
Author
|
I can confirm uv doesn't affect GTSAM. I successfully compiled and ran both the Python and Matlab tests. |
dellaert
approved these changes
Nov 29, 2025
Member
dellaert
left a comment
dellaert
left a comment
There was a problem hiding this comment.
Awesome, thanks for this heroic effort!
17404733e Merge pull request #180 from borglab/fix-175 2b5eaea46 fixes 150c09506 update tests b1041a31a better repr to avoid regression eb94093e6 fix scoped template issue 53c67028c improve Typename repr f6dc1750d refactor Typename constructor c4c98d5ee use f-strings in helpers.py 1278e499f refactor multilevel_instantiation to be more pythonic 416c23c02 rename ret to result 3b66b898a declare return type of instantiate_methods 6ef12575c formatting 6e918eeb1 rename union types to be singular e688e47bd fix scoped template bug git-subtree-dir: wrap git-subtree-split: 17404733ea315bd8c6cb06341eb153064e859794
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After a whole week of debugging and verifying, I was able to fix the Matlab wrapper.
There were multiple PRs involved which resulted in this breaking, so there needs to be a stronger requirement from contributors who modify the wrapper or interface files to verify their changes work across both Python and Matlab.