perf(linear): compute JacobianFactor::gradient() directly, skip HessianFactor conversion#2510
Conversation
…sianFactor
JacobianFactor::gradient(Key, VectorValues) was converting the entire
factor to a HessianFactor (O(n*m^2) for A'A computation) just to read
one gradient entry. This was flagged by a TODO in the code.
The gradient for key k is A_k' * Sigma^{-1} * (A*x - b), which can be
computed in O(n*m) by evaluating the residual directly and multiplying
by the transpose of the requested block. This avoids the HessianFactor
allocation and the full Hessian matrix product.
Adds two tests verifying numerical equivalence with the HessianFactor
path, with and without a noise model.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR optimizes JacobianFactor::gradient(Key, VectorValues) by computing a single-key gradient directly from Jacobian blocks, avoiding conversion to HessianFactor and its AᵀA construction cost.
Changes:
- Reimplemented
JacobianFactor::gradient()to compute the residual and project onto a requested key’s Jacobian block. - Added unit tests comparing the new path to the existing HessianFactor-based gradient for isotropic and no-noise cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gtsam/linear/JacobianFactor.cpp | Replaces HessianFactor conversion with direct residual/gradient computation. |
| gtsam/linear/tests/testJacobianFactor.cpp | Adds tests validating gradient equivalence against HessianFactor. |
The previous implementation applied whitenInPlace twice, computing R*R*e instead of R^T*R*e. These differ for non-symmetric R (any non-diagonal covariance). Fix: whiten both A_k and e once, then compute A_k_w^T * e_w = A_k^T * R^T * R * e. Also added bounds check for missing key and a test with non-diagonal covariance.
|
Pushed a fix for the noise model handling. The previous version applied Also added a bounds check for missing keys and a test with non-diagonal (Gaussian) covariance that would have caught this. |
|
@jashshah999 please "resolve" copilot comments you addressed and/or are irrelevant. |
|
Resolved all Copilot threads (replied to each). Also merged develop to re-trigger CI. |
dellaert
left a comment
There was a problem hiding this comment.
Very nice. We'll merge when CI passes.
Problem
JacobianFactor::gradient(Key, VectorValues)converts the entire factor to aHessianFactorto compute a single gradient entry (flagged by an existing TODO at line 881):The
HessianFactorconstructor computesA'Awhich is O(n*m^2) for an n-row, m-column Jacobian. This is unnecessary when only one key's gradient is needed.Fix
Compute the gradient directly:
e = A*x - bby iterating over Jacobian blocks — O(n*m)e = Sigma^{-1} * e(double whiten)A_k' * efor the requested key — O(n*d_k)Total cost: O(nm) vs O(nm^2) for the old path. No heap allocation for the HessianFactor.
Tests
Two new tests in
testJacobianFactor.cpp:gradient: isotropic noise model, verifies numerical equivalence with the HessianFactor pathgradient_no_noise: no noise model (unit covariance), same verification