feat(isam2): adaptive variable reordering triggered by fill-in growth by jashshah999 · Pull Request #2509 · borglab/gtsam

jashshah999 · 2026-04-27T05:48:44Z

Problem

In long-running iSAM2 sessions the Bayes tree accumulates fill-in because the variable ordering is only refreshed for the affected subtree on each incremental update. Over thousands of steps the total number of nonzero entries (nnz) in the tree grows beyond what a fresh COLAMD ordering would produce, degrading both memory usage and solve time.

The existing batch fallback (triggered when >65% of variables are affected) only fires on large loop closures. Gradual fill-in accumulation from incremental updates goes undetected.

Solution

Track the Bayes tree nnz after each full reorder and automatically trigger a batch re-elimination with fresh COLAMD ordering when fill-in grows past a configurable threshold.

New parameters in ISAM2Params:

enableAdaptiveReorder (default: false) -- opt-in, no behavior change for existing users
adaptiveReorderThreshold (default: 2.0) -- trigger batch reorder when current nnz exceeds this multiple of the baseline nnz

New fields in ISAM2Result:

treeNnz -- total Bayes tree nnz, reported every update regardless of whether adaptive reorder is enabled, useful for monitoring fill-in
batchReorderTriggered -- flag indicating a reorder happened on this step

New method on ISAM2:

treeNnz() -- compute total nnz across all cliques (uses existing ISAM2Clique::nnz_internal)

How it works

After the first update(), the current nnz is recorded as the baseline
Before each subsequent recalculate(), if enableAdaptiveReorder is true, the current nnz is compared to adaptiveReorderThreshold * baseline
If the ratio is exceeded, all keys are marked so that recalculate() takes the existing recalculateBatch() path (full COLAMD reorder + re-elimination)
After any batch reorder (adaptive or from the existing 65% threshold), the baseline is refreshed

No new algorithms -- this reuses the existing recalculateBatch() infrastructure. The only new computation is treeNnz(), which is O(number of cliques).

Tests

Three new tests in testGaussianISAM2.cpp:

AdaptiveReorder_NnzTracking: verifies nnz is populated after first update and grows with added factors
AdaptiveReorder_Triggered: uses threshold=1.01 with a loop closure to exercise the reorder path
AdaptiveReorder_DisabledByDefault: verifies no reorder with default params

In long-running iSAM2 sessions the Bayes tree accumulates fill-in because the variable ordering is only refreshed for the affected subtree on each incremental update. Over thousands of steps the total number of nonzero entries (nnz) in the tree can grow well beyond what a fresh COLAMD ordering would produce, degrading both memory and solve time. This commit adds: - ISAM2::treeNnz() — compute total nnz across all cliques - ISAM2Params::enableAdaptiveReorder (default: false) — opt-in flag - ISAM2Params::adaptiveReorderThreshold (default: 2.0) — ratio of current nnz to baseline nnz that triggers a full batch re-elimination with fresh COLAMD ordering (reuses the existing recalculateBatch path) - ISAM2Result::treeNnz — reported every update for monitoring - ISAM2Result::batchReorderTriggered — flag indicating a reorder happened The baseline nnz is recorded after each batch reorder (including the first update and whenever the existing 65% batch threshold fires). When the current nnz exceeds threshold * baseline, all keys are marked so that recalculate() takes the batch path on the current step. Disabled by default so there is no behavior change for existing users. Three new tests in testGaussianISAM2.cpp: nnz tracking, triggered reorder with a low threshold, and verification that the feature is off by default.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an opt-in “adaptive reorder” heuristic for iSAM2 that monitors Bayes tree fill-in (nnz) over time and triggers a full batch reorder when nnz grows beyond a configurable threshold.

Changes:

Add treeNnz reporting and batchReorderTriggered signaling in ISAM2Result.
Add adaptive reorder knobs to ISAM2Params and implement ISAM2::treeNnz() plus reorder triggering logic in ISAM2::update().
Add unit tests covering nnz tracking, reorder triggering behavior, and default-disabled behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/testGaussianISAM2.cpp	Adds tests for nnz tracking and adaptive reorder behavior.
gtsam/nonlinear/ISAM2Result.h	Extends the update result with nnz telemetry + reorder flag and prints/getters.
gtsam/nonlinear/ISAM2Params.h	Introduces new adaptive reorder parameters, defaults, and print output.
gtsam/nonlinear/ISAM2.h	Adds stored baseline nnz state and exposes `treeNnz()` API.
gtsam/nonlinear/ISAM2.cpp	Implements `treeNnz()` and adaptive reorder triggering + baseline refresh logic.

Comments suppressed due to low confidence (1)

gtsam/nonlinear/ISAM2.cpp:1

ISAM2::equals does not include the newly added nnzAfterLastReorder_. Since this value influences future adaptive reorder decisions, two instances can be ‘equal’ per this method while exhibiting different behavior on subsequent update() calls. Consider including nnzAfterLastReorder_ in the comparison (or document clearly why it is intentionally excluded and safe to ignore).

/* ----------------------------------------------------------------------------

jashshah999 · 2026-04-30T21:14:23Z

+  // With threshold 1.01, the loop closure fill-in should trigger a reorder
+  // on this or the next update
+  bool triggered = result.batchReorderTriggered;
+  if (!triggered) {
+    // Try one more update to see if fill-in was detected
+    graph = NonlinearFactorGraph();
+    graph.emplace_shared<BetweenFactor<Pose2>>(5, 10,
+        Pose2(5.0, 0.0, 0.0), odoNoise);
+    result = isam.update(graph, init);
+    triggered = result.batchReorderTriggered;
+  }
+
+  // At some point the reorder should have triggered
+  // (If not, the nnz didn't grow — which is also fine, means no fill-in)
+  EXPECT(result.treeNnz > 0);


Fixed. Added EXPECT(triggered) assertion.

jashshah999 · 2026-04-30T21:14:25Z

+  // 8. Check whether adaptive reorder should force a batch recalculation.
+  // If fill-in has grown past the threshold, mark all keys so that
+  // recalculate() takes the batch path.
+  if (params_.enableAdaptiveReorder && nnzAfterLastReorder_ > 0 &&
+      !roots_.empty()) {
+    const size_t currentNnz = treeNnz();
+    if (currentNnz >
+        static_cast<size_t>(params_.adaptiveReorderThreshold *
+                            static_cast<double>(nnzAfterLastReorder_))) {
+      for (const auto& [key, _] : variableIndex_) {
+        result.markedKeys.insert(key);
+      }
+      result.batchReorderTriggered = true;
+    }
+  }


Fixed. Flag is now set in both the adaptive and 65% threshold paths.

jashshah999 · 2026-04-30T21:14:26Z

+  // Update baseline when: first update, adaptive reorder fired, or the
+  // existing batch threshold was hit (variablesReeliminated == all vars).
+  if (nnzAfterLastReorder_ == 0 || result.batchReorderTriggered ||
+      result.variablesReeliminated >= theta_.size()) {


Fixed. Using batchReorderTriggered flag directly.

jashshah999 · 2026-04-30T21:14:28Z

+    if (currentNnz >
+        static_cast<size_t>(params_.adaptiveReorderThreshold *
+                            static_cast<double>(nnzAfterLastReorder_))) {


Fixed. Using floating-point ratio comparison.

dellaert · 2026-04-27T23:02:28Z

Could you clarify how the threshold is set in the PR comments and ( if needed) in the comments in the code? The PR comment makes reference to a “baseline” NNZ but it’s unclear how that is obtained.

jashshah999 · 2026-04-28T00:16:11Z

The baseline nnz is recorded after each full batch re-elimination — the first update() sets it (fresh tree), and it's refreshed whenever recalculateBatch() runs (either from this heuristic or the existing 65% threshold). The check then triggers when currentNnz > adaptiveReorderThreshold * baseline.

Happy to add a clarifying comment in the code if you'd like.

jashshah999 · 2026-04-30T03:16:40Z

To clarify the threshold: the baseline NNZ is captured once during the first full batch elimination, then refreshed after each subsequent batch reorder. The nnzRatio parameter (default 2.0) triggers reorder when current fill-in exceeds 2x the baseline. Happy to add a code comment making this clearer.

dellaert · 2026-04-30T12:40:16Z

Hi @jashshah999 . Yes, please make this clear in the code. Also, please evaluate the copilot comments above for accuracy, and “resolve” all of them by either addressing or deeming them irrelevant. They often point to real issues.

- Set batchReorderTriggered in the existing 65% threshold path too, matching the ISAM2Result documentation - Use floating-point ratio comparison instead of size_t cast to avoid truncation for large baselines - Use batchReorderTriggered flag for baseline refresh instead of the brittle variablesReeliminated proxy - Add EXPECT(triggered) assertion in test

jashshah999 · 2026-04-30T21:13:59Z

Addressed all four copilot comments:

Added EXPECT(triggered) assertion in the test
batchReorderTriggered is now set in the 65% threshold path too, matching the ISAM2Result docs
Baseline refresh uses the batchReorderTriggered flag directly instead of the variablesReeliminated >= theta_.size() proxy
Threshold comparison uses floating-point ratio to avoid size_t truncation

Also added a comment clarifying the baseline NNZ mechanism.

dellaert requested a review from Copilot April 27, 2026 20:47

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Copilot started reviewing on behalf of dellaert April 27, 2026 22:55 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(isam2): adaptive variable reordering triggered by fill-in growth#2509

feat(isam2): adaptive variable reordering triggered by fill-in growth#2509
jashshah999 wants to merge 2 commits intoborglab:developfrom
jashshah999:feat/adaptive-reorder

jashshah999 commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jashshah999 Apr 30, 2026

Uh oh!

jashshah999 Apr 30, 2026

Uh oh!

jashshah999 Apr 30, 2026

Uh oh!

jashshah999 Apr 30, 2026

Uh oh!

dellaert commented Apr 27, 2026

Uh oh!

jashshah999 commented Apr 28, 2026

Uh oh!

jashshah999 commented Apr 30, 2026

Uh oh!

dellaert commented Apr 30, 2026

Uh oh!

jashshah999 commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jashshah999 commented Apr 27, 2026

Problem

Solution

How it works

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jashshah999 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

jashshah999 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

jashshah999 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

jashshah999 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

dellaert commented Apr 27, 2026

Uh oh!

jashshah999 commented Apr 28, 2026

Uh oh!

jashshah999 commented Apr 30, 2026

Uh oh!

dellaert commented Apr 30, 2026

Uh oh!

jashshah999 commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants