Fix IncrementalFixedLagSmoother with factor removal#2474
Open
Ellon wants to merge 6 commits intoborglab:developfrom
Open
Fix IncrementalFixedLagSmoother with factor removal#2474Ellon wants to merge 6 commits intoborglab:developfrom
Ellon wants to merge 6 commits intoborglab:developfrom
Conversation
Ellon
force-pushed
the
fix_incremental_fixed_lag_smoother_with_removal
branch
from
2c74c5a to
688a246
Compare
Collaborator
|
@Ellon can you first open two issues on the bug you discovered and link this PR? |
dellaert
reviewed
Mar 20, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect behavior in IncrementalFixedLagSmoother/ISAM2 when factors are removed such that some keys become unused, and adds a regression test that exercises frequent updates with periodic factor removal.
Changes:
- Add a new regression test covering repeated updates with
factorsToRemove, including unused-key removal scenarios. - Update
IncrementalFixedLagSmootherto erase timestamps for keys reported asunusedKeysafter anISAM2::update(). - Fix
ISAM2::recalculateBatch()to excludeunusedKeysfrom constrained ordering groups.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gtsam/nonlinear/tests/testIncrementalFixedLagSmoother.cpp | Adds a new test scenario that removes factors to intentionally create unused keys. |
| gtsam/nonlinear/IncrementalFixedLagSmoother.cpp | Removes timestamps for keys that become unused after an update. |
| gtsam/nonlinear/ISAM2.cpp | Filters ordering constraints during batch recalculation to exclude unused keys. |
This was referenced Mar 23, 2026
Contributor
Author
Contributor
Author
|
I'm updating the test to expose the problem mentioned by copilot. I'll update the branch in a few minutes. |
... related to values becoming unused when factors are removed during an update.
…e during an update
Ellon
force-pushed
the
fix_incremental_fixed_lag_smoother_with_removal
branch
from
357a2c3 to
6592581
Compare
Contributor
Author
|
Done. Now the test expose the bug found by copilot. @ProfFan Do I need to open another issue? IMO I think all these issues are in fact one single issue related to proper handling of factor removals in the smoother. |
…talFixedLagSmoother
Ellon
force-pushed
the
fix_incremental_fixed_lag_smoother_with_removal
branch
from
6592581 to
488c5ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
I was working with the
IncrementalFixedLagSmootherin a setup where the smoother is updated frequently with new values, but only a subset of these values are kept in the long term. The values are removed by removing all the factors connecting them to the rest of the graph, using the argumentfactorsToRemoveof the smoother'supdate(...)method.While at it I found two bugs related to factor removals and unused values. In this PR I added a test that reproduces my setup and exposes the bugs, and two other commits fixing each one of them.
I hope it is good enough for a merge, otherwise let me know what can be improved.
Cheers,
EDIT: force-pushed changes to simplify the test file.