Skip to content

Allowing Non Noise Model Factors for Gnc#2410

Merged
dellaert merged 8 commits intoborglab:developfrom
hkhanuja:fix/gncoptimizer
Feb 18, 2026
Merged

Allowing Non Noise Model Factors for Gnc#2410
dellaert merged 8 commits intoborglab:developfrom
hkhanuja:fix/gncoptimizer

Conversation

@hkhanuja
Copy link
Copy Markdown

@hkhanuja hkhanuja commented Feb 16, 2026

This PR makes sure that non NoiseModel factors do not throw errors during initialization for Gnc Optimization.
Earlier in the GncOptimizer initialization, if the factor is not a NoiseModelFactor, then the line graph.at<NoiseModelFactor>(i) returns a null pointer, which later causes a segmentation fault.
In this PR, we add fallbacks so that instead of a segmentation fault, those factors are considered as known inliers, and while they contribute to the loss, their weights are fixed.

Comment thread gtsam/nonlinear/GncOptimizer.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

This PR updates GncOptimizer initialization to tolerate factors that are not NoiseModelFactors, preventing failures when such factors are present in the graph.

Changes:

  • Detect and bypass non-NoiseModelFactor entries during graph normalization and subsequent GNC computations.
  • Exclude non-NoiseModelFactors from GNC thresholding (barcSq_) and mu_init initialization logic.
  • Normalize knownInliers (sort/unique) before later set-style operations.

Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
@hkhanuja hkhanuja changed the title Allowing Non Noise Model Factors Allowing Non Noise Model Factors for Gnc Feb 16, 2026
@dellaert
Copy link
Copy Markdown
Member

Based on a very quick review with Akshay, I agree that modifying the params after they've been set is not the right way to go. Something better would be a parameter setting saying, "Treat non-noise model factors as inliers." And this is then executed by the optimizer, but not by modifying the params struct, which is typically presumed constant after construction. Then if that parameter is not set - and I would set it by default as false- the optimizer simply throws an exception if it encounters non-noise model factors - and the exception message would point to the existence of the flag :-).

Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
Comment thread tests/testGncOptimizer.cpp
Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Unfortunately, I feel that this one is not ready to merge yet. Dynamic pointer casts in C++ are quite expensive. And now we're adding several of them in an inner loop - for a rare use case. In addition, I found that there was one in the original code that could be replaced with a static cast.

I think a workable solution is put all the non-noise model factors at the end in nfg_: then in the loops over the nfg_, which are all size_t indexed, we just have to treat the last nrNonNoiseModelFactors_ differently. Then we also need only one new member variable: nrNonNoiseModelFactors_.

Comment thread gtsam/nonlinear/GncParams.h Outdated
Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
Comment thread gtsam/nonlinear/GncOptimizer.h Outdated
@hkhanuja hkhanuja requested a review from dellaert February 18, 2026 02:45
Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Nice !

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.

4 participants