Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new NonlinearMultifrontalSolver class that integrates the recently-added multifrontal solver into the nonlinear optimization pipeline, enabling significant performance improvements for bundle adjustment and SLAM problems. The new solver becomes the default linear solver for nonlinear optimizers, replacing the legacy multifrontal Cholesky implementation.
Changes:
- Introduced
NonlinearMultifrontalSolverwrappingMultifrontalSolverwith LM-style damping support - Changed default linear solver from
MULTIFRONTAL_CHOLESKYtoMULTIFRONTAL_SOLVER - Refactored damping parameters into standalone
LMDampingParamsstruct and extractedMultifrontalParameters - Added
deltaError()support toMultifrontalSolverfor efficient error computation - Enhanced
MultifrontalCliquewith damped elimination methods supporting both identity and diagonal damping - Integrated new solver into
LevenbergMarquardtOptimizerandGaussNewtonOptimizerwith automatic fallback
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| gtsam/nonlinear/NonlinearMultifrontalSolver.h/.cpp | New solver class wrapping MultifrontalSolver with nonlinear-specific functionality |
| gtsam/nonlinear/LMDampingParams.h | Extracted damping parameters into standalone header |
| gtsam/nonlinear/NonlinearOptimizerParams.h/.cpp | Added MULTIFRONTAL_SOLVER enum and MultifrontalParameters field |
| gtsam/nonlinear/NonlinearOptimizer.h/.cpp | Added ensureMultifrontalSolver method for lazy solver initialization |
| gtsam/nonlinear/LevenbergMarquardtParams.h/.cpp | Refactored to use LMDampingParams struct |
| gtsam/nonlinear/LevenbergMarquardtOptimizer.cpp | Integrated new solver with conditional usage based on params |
| gtsam/nonlinear/GaussNewtonOptimizer.cpp | Added support for new multifrontal solver |
| gtsam/nonlinear/Values.h | Added friend declaration for efficient value updates |
| gtsam/linear/MultifrontalParameters.h | New header for multifrontal solver parameters |
| gtsam/linear/MultifrontalSolver.h/.cpp | Added deltaError method and refactored parameter handling |
| gtsam/linear/MultifrontalClique.h/.cpp | Enhanced with damped elimination, QR mode determination, and error tracking |
| gtsam/nonlinear/tests/testNonlinearMultifrontalSolver.cpp | Comprehensive tests for new solver functionality |
| tests/testNonlinearOptimizer.cpp | Updated to test all solver types including new MULTIFRONTAL_SOLVER |
| gtsam/linear/tests/testMultifrontalSolver.cpp | Added deltaError tests and improved comment formatting |
| gtsam/base/tests/testSymmetricBlockMatrix.cpp | Added tests for diagonal update helpers |
| timing/timeSFMBAL.cpp | Refactored timing harness to compare solvers and support profiling |
| timing/timeNonlinearMultifrontalSolver.cpp | New timing tool for direct solver benchmarking |
|
Just out of curiosity what are the major changes that give the improved performance? (maybe you have already written some explanation and you can just point me to it?) |
| void initializeMatrices(const std::vector<size_t>& blockDims, | ||
| size_t totalNumRows); | ||
| /// Apply damping for QR elimination by writing into extra Ab_ rows. | ||
| void applyDampingQR(double lambda, const LMDampingParams& dampingParams, |
There was a problem hiding this comment.
Could you add a context section on why and how we need the extra rows?
ProfFan
left a comment
ProfFan
left a comment
There was a problem hiding this comment.
Nice! Let's merge this. I just moved to my new place and setting things up, but I will start implementing the TODOs now :)
@tzvist Please see #2350 - I just added a short description to what we're trying to do. It seems that you have a compute-heavy use case and we would love to have your help in making this even better; probably over preferring this over optimizing the earlier pipeline :-) |
Schur |
|
By the way, I'm dealing with some regressions. The new solver needs a non-null noise model to do the symbolic analysis. So I will disallow null pointers. I'm doing this in a different branch and I will target that branch after I have a new PR up. |
This is the big one. This uses the new multifrontal solver in the nonlinear optimizer, by means of a derived class
NonlinearMultifrontalSolver.Refer to copilot summary for the detailed changes, but here are the perf numbers, for BAL (SLAM-like datasets likely have much greater wins, but I did not run those yet):
TBB On Linux
TaskScheduler on Linux
TBB On M1 Macbook
TaskScheduler on M1
Across both Linux and M1 macOS, the new TaskScheduler consistently outperforms the legacy Cholesky nonlinear optimizer and matches or exceeds TBB speedups, with the largest gains on smaller and mid-sized BAL problems (≈2.0–2.4×), while improvements taper to ≈1.25–1.9× on the largest Dubrovnik instance.
TaskScheduler closes much of the performance gap to TBB, but TBB still sets the raw-performance ceiling, especially at large scale (135 cameras).
Sample run (Mac, no TBB)