Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/1452 #1977

Merged
merged 9 commits into from
Jan 21, 2025
Merged

Fix/1452 #1977

merged 9 commits into from
Jan 21, 2025

Conversation

JokerJohn
Copy link
Contributor

Add a minimal test case of IncrementalFixedLagSmoother in gtsam/gtsam_unstable/examples folder using real-world data to showcase how setting findUnusedFactorSlots = true addresses the issue #1452 in GTSAM.

Copy link
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.

I already added the text file you provided in examples/Data/. Is the file you provided different?

*/
int main() {
// Path to the test data file
ifstream infile("IncrementalFixedLagSmootherTestData.txt");
Copy link
Member

Choose a reason for hiding this comment

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

Could you load the file as in the test I provided in this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

saveG2oGraph(smoother.getFactors(), currentEstimate, path + "isam", lineCount);
infile.close();
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Add a new line at end of file, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

@dellaert
Copy link
Member

You’re merging this into the branch I created, which added this as a unit test. I don’t think we need both, so could you also delete the test in this PR? Also look at how I load the file, and perhaps how I now use emplace_shared.

@JokerJohn
Copy link
Contributor Author

You’re merging this into the branch I created, which added this as a unit test. I don’t think we need both, so could you also delete the test in this PR? Also look at how I load the file, and perhaps how I now use emplace_shared.

Sorry, I just noticed that you have already provided the data file issue1452.txt in the Data directory. I have moved this example to the appropriate folder and modified the corresponding file reading and factor adding code.

@dellaert
Copy link
Member

You’re merging this into the branch I created, which added this as a unit test. I don’t think we need both, so could you also delete the test in this PR? Also look at how I load the file, and perhaps how I now use emplace_shared.

Ping on this comment :-)

@JokerJohn JokerJohn closed this Jan 17, 2025
@JokerJohn JokerJohn deleted the fix/1452 branch January 17, 2025 16:16
@dellaert
Copy link
Member

dellaert commented Jan 17, 2025

Hi @JokerJohn - I actually liked your example - I was asking to remove the test :-)
Feel free to re-open

@JokerJohn
Copy link
Contributor Author

Hi @JokerJohn - I actually liked your example - I was asking to remove the test :-) Feel free to re-open

Sorry,I saw the unit test code for issue 1452, which has already incorporated the example code, so I closed this PR. If there's a need to change it into an example case, I can reopen this PR.

@JokerJohn JokerJohn reopened this Jan 18, 2025
@dellaert
Copy link
Member

Excellent, but could you then remove the (slow and redundant) unit test that basically does the same? Your example shows (a) it works, (b) a useful example.

@JokerJohn
Copy link
Contributor Author

Excellent, but could you then remove the (slow and redundant) unit test that basically does the same? Your example shows (a) it works, (b) a useful example.

done.

@dellaert dellaert changed the base branch from fix/1452 to develop January 20, 2025 20:26
Copy link
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.

Cool!

@varunagrawal varunagrawal merged commit c3908c8 into borglab:develop Jan 21, 2025
33 checks passed
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.

3 participants