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

Wrap additional ISAM2 methods #941

Merged
merged 11 commits into from
Nov 30, 2021
Merged

Wrap additional ISAM2 methods #941

merged 11 commits into from
Nov 30, 2021

Conversation

varunagrawal
Copy link
Collaborator

Wrapping additional methods in ISAM2 needed for legged state estimation.

@varunagrawal varunagrawal added the quick-review Quick and easy PR to review label Nov 27, 2021
@varunagrawal varunagrawal self-assigned this Nov 27, 2021
@varunagrawal varunagrawal added the high-priority Need this done quickly label Nov 29, 2021
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.

Confused. Most additions are commented out. Make draft PR until finalized.

@varunagrawal
Copy link
Collaborator Author

Okay removed out the commented stuff. That was my mistake, I added those in as part of another change not related to this PR. Thank you for catching them. 🙂

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.

LGTM, but is that the only method you want to add? Are there others public methods that could easily be added in the same PR?

@varunagrawal
Copy link
Collaborator Author

No other methods so far that we need for MHS + State Estimator. If we find something, we can make a PR for then.

@dellaert
Copy link
Member

My question was not about MHS, but expanding this PR to include other methods now that are easily wrapped.

@varunagrawal
Copy link
Collaborator Author

We can. It just depends on the requirements. My comment about MHS is that the current wrapped method is all I need so I didn't spend too much time looking at other methods.

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.

OK, you're (choosing) to not hear me :-) For this GTSAM PR, please also wrap any other easy wrap-able functions before merging, so GTSAM users have this convenience in the future.

@varunagrawal
Copy link
Collaborator Author

Gotcha

@dellaert
Copy link
Member

Cool, thanks !!!

@varunagrawal varunagrawal merged commit 7bd4ebf into develop Nov 30, 2021
@varunagrawal varunagrawal deleted the wrap/isam2 branch November 30, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority Need this done quickly quick-review Quick and easy PR to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants