Skip to content

[codex] Add copy-free matrix view hooks#184

Merged
dellaert merged 3 commits into
masterfrom
codex/py-arg-policy-hook
May 26, 2026
Merged

[codex] Add copy-free matrix view hooks#184
dellaert merged 3 commits into
masterfrom
codex/py-arg-policy-hook

Conversation

@dellaert
Copy link
Copy Markdown
Member

@dellaert dellaert commented May 26, 2026

Summary

  • Add a generated gtwrap::internal::PyArgPolicy<T> / py_arg<T>(name) hook for pybind argument declarations.
  • Keep the default Python behavior equivalent to pybind11::arg(name) so existing wrappers continue to use normal pybind conversion.
  • Route generated typed Python arguments, including defaulted arguments, through the hook so downstream preambles can specialize policies such as .noconvert().
  • Restrict the recent reference_internal return policy to const reference method returns, avoiding it for mutable references.
  • Add MATLAB wrapper support for ConstMatrixView-style matrix view arguments by accepting MATLAB double matrices and generating unwrapMatrixView<T>(...) locals backed by the mxArray buffer.

Why

Downstream projects need a wrap-level extension point for copy-free matrix-view arguments without making wrap depend on project-specific Python CLI configuration. This lets GTSAM specialize Python matrix-view arguments in a preamble and lets MATLAB pass compatible double matrices into const Eigen matrix views without the existing unwrap<Matrix> copy.

Impact

Ordinary Python and MATLAB arguments keep their existing behavior. ConstMatrixView arguments are treated as MATLAB double matrix inputs and are passed through generated wrappers as value-like view objects, not pointer/shared-pointer objects.

Validation

  • conda run -n py312 python -m unittest discover tests
  • git diff --check
  • git diff --cached --check
  • MATLAB R2025b smoke compile: /Applications/MATLAB_R2025b.app/bin/mex ... /tmp/gtwrap_matrix_view_mex.cpp -outdir /tmp
  • MATLAB R2025b runtime smoke: matlab -batch "addpath(/tmp); y = gtwrap_matrix_view_mex([1 3 5; 2 4 6]); assert(y == 2323); disp(y);"

@dellaert dellaert requested review from ProfFan and Copilot May 26, 2026 18:52
@dellaert dellaert marked this pull request as ready for review May 26, 2026 18:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an extension point for pybind11 argument declarations by generating typed arguments through a gtwrap::internal::PyArgPolicy<T> / py_arg<T>(name) hook (defaulting to pybind11::arg(name)), and tightens reference_internal return policy emission to only apply to const-reference method returns.

Changes:

  • Generate gtwrap::internal::PyArgPolicy + py_arg<T>(name) support code into produced pybind C++ and route generated named arguments (including defaulted args) through py_arg<T>(...).
  • Update expected generated pybind outputs to reflect py_arg<T>(...) usage in .def(...) declarations.
  • Add/extend unit tests and fixtures for the new argument policy hook and for restricting reference_internal to const-reference method returns.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
gtwrap/pybind_wrapper.py Implements the argument-policy hook injection and switches named argument emission to py_arg<T>(...); restricts reference_internal to const-ref method returns.
tests/test_pybind_wrapper.py Adds coverage for the new arg-policy hook and the refined const-ref return policy behavior; allows passing a custom module template for targeted assertions.
tests/fixtures/arg_policies.i New fixture exercising typed args + defaulted args for policy hook generation.
tests/fixtures/return_policies.i New fixture validating reference_internal is emitted only for const-ref method returns.
tests/expected/python/*_pybind.cpp Updates golden expected pybind C++ outputs to use gtwrap::internal::py_arg<T>(...) and include the generated hook support block.

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

@dellaert dellaert changed the title [codex] Add pybind argument policy hook [codex] Add copy-free matrix view hooks May 26, 2026
@dellaert dellaert marked this pull request as draft May 26, 2026 19:06
@dellaert dellaert requested a review from Copilot May 26, 2026 19:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Comment thread matlab.h Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

@dellaert
Copy link
Copy Markdown
Member Author

I tested this on my copy of GTSAM and it works like a charm. I will merge and vendor in this new version of wrap and monitor the situation on the GTSAM CI.

@dellaert dellaert marked this pull request as ready for review May 26, 2026 19:41
@dellaert dellaert merged commit d4b1871 into master May 26, 2026
9 checks passed
@dellaert dellaert deleted the codex/py-arg-policy-hook branch May 26, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants