Skip to content

Conversation

ProfFan
Copy link
Collaborator

@ProfFan ProfFan commented Oct 7, 2025

About 10% faster in linearization with my microbenchmarks. I think there is still a large margin for optimization, but we should focus more on the linear solver...

@dellaert dellaert requested review from Copilot and dellaert October 7, 2025 19:27
Copy link
Contributor

@Copilot 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

This PR introduces a new FastNoiseModelFactorN class as an optimized alternative to the existing NoiseModelFactorN for more efficient linearization in GTSAM. The implementation aims to achieve approximately 10% performance improvement in linearization operations.

  • Complete implementation of FastNoiseModelFactorN with optimized linearization using VerticalBlockMatrix
  • Migration of PriorFactor to use the new fast factor implementation
  • API adjustments to support Eigen::Ref-based Jacobian handling for better memory management

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
gtsam/nonlinear/FastNoiseModelFactorN.h New optimized factor implementation with direct VerticalBlockMatrix linearization
gtsam/nonlinear/PriorFactor.h Migration from NoiseModelFactorN to FastNoiseModelFactorN base class
gtsam/slam/tests/testPriorFactor.cpp Updated test to use new evaluateError signature with nullptr parameter
gtsam/linear/NoiseModel.h API changes to support Eigen::Block instead of Eigen::Block
gtsam/linear/JacobianFactor.h Simplified constructor template for better move semantics
gtsam/linear/JacobianFactor-inl.h Unified constructor implementation with perfect forwarding
gtsam/linear/GaussianConditional.cpp Optimization to use std::move for VerticalBlockMatrix
gtsam/navigation/ImuBias.cpp Added .eval() calls to fix potential Eigen expression template issues
gtsam/base/OptionalJacobian.h New constructor overload for Eigen::Ref support

namespace gtsam {
/* ************************************************************************* */
/**
* A convenient base class for creating your own NoiseModelFactor
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

Documentation should be updated to reflect that this is a FastNoiseModelFactor, not NoiseModelFactor.

Suggested change
* A convenient base class for creating your own NoiseModelFactor
* A convenient base class for creating your own FastNoiseModelFactor

Copilot uses AI. Check for mistakes.

* between a Pose3 and Point3 could be implemented like so:
*
* ~~~~~~~~~~~~~~~~~~~~{.cpp}
* class MyFactor : public FastNoiseModelFactorN<Pose3, Point3> {
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

[nitpick] The example class name should follow the naming convention with a more descriptive name than 'MyFactor'.

Copilot uses AI. Check for mistakes.

template <typename T = void>
using OptionalMatrixTypeT = Matrix*;

/* Like std::void_t, except produces `Eigen::Ref<Matrix>` instead of `void`.
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

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

The comment is misleading - this template produces Eigen::Ref<Matrix>* (pointer), not Eigen::Ref<Matrix>.

Suggested change
/* Like std::void_t, except produces `Eigen::Ref<Matrix>` instead of `void`.
/* Like std::void_t, except produces `Eigen::Ref<Matrix>*` (pointer) instead of `void`.

Copilot uses AI. Check for mistakes.

@dellaert
Copy link
Member

dellaert commented Oct 7, 2025

Fan, I hope you saw that the CI fails. This could be due to unrelated changes you made. Maybe those should be a separate PR :-)

i’m wondering whether it is necessary to have a completely new class or whether we can just have a different method to overload. Then we can deprecate the old method and publicize the new method. And this PR would also have much smaller change and be easier to review.

@ProfFan ProfFan force-pushed the fan/fast_nm_factor branch from b6908be to 775ce42 Compare October 8, 2025 01:32
@ProfFan ProfFan force-pushed the fan/fast_nm_factor branch from 775ce42 to 374632c Compare October 8, 2025 02:03
@dellaert
Copy link
Member

@ProfFan Let's talk Monday about my comment and whether that's a good or a bad idea, and why...

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