Skip to content

Clarify JointMarginal::at()/operator() indexing semantics (Key, not position)#2592

Merged
dellaert merged 2 commits into
borglab:developfrom
thatdudegrantt:fix-1305-jointmarginal-docs
Jul 7, 2026
Merged

Clarify JointMarginal::at()/operator() indexing semantics (Key, not position)#2592
dellaert merged 2 commits into
borglab:developfrom
thatdudegrantt:fix-1305-jointmarginal-docs

Conversation

@thatdudegrantt

Copy link
Copy Markdown
Contributor

Summary
JointMarginal::at(iVariable, jVariable) (and its operator() equivalent) in gtsam/linear/JointMarginal.h takes the actual nonlinear Keys used to construct the JointMarginal, not positional/row indices into the underlying block matrix. This wasn't documented, and it caused confusion in #1305, where a user compared joint_covs.at(0, key) against marginals.marginalCovariance(key) expecting them to match. They don't, and at(0, key) returns the cross-covariance block between variable 0 and variable key, not the diagonal (self-covariance) block for key. The correct call for that comparison is at(key, key).

This PR is a comment-only clarification of the existing behavior. No functional/behavioral changes, as operator()and at() are untouched apart from the docstrings.
Fixes #1305

Change
Updated the doc comments on JointMarginal::operator() and JointMarginal::at() in gtsam/linear/JointMarginal.h to explicitly state that arguments are Keys (not indices), and that at(key, key) is the call equivalent to Marginals::marginalCovariance(key).

Testing
Documentation-only change, so no code paths affected and no new tests are needed. I also verified the existing unit tests (testMarginals) still pass unmodified, confirming no behavioral change.

Environment
OS: Windows 11 x64
Compiler: Visual Studio 2026 Community, MSVC 19.51
CMake: 4.2.3

@dellaert dellaert merged commit a3b8a34 into borglab:develop Jul 7, 2026
37 of 40 checks passed
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.

Is jointMarginalCovariance behaving as expected?

2 participants