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

Hybrid ISAM #1273

Merged
merged 6 commits into from
Aug 22, 2022
Merged

Hybrid ISAM #1273

merged 6 commits into from
Aug 22, 2022

Conversation

varunagrawal
Copy link
Collaborator

This PR gets the last non-trivial test for Hybrid GaussianISAM to pass, and adds necessary updates to avoid crashes and incorrect results.

After this, hybrid elimination should be good to go!

@varunagrawal varunagrawal self-assigned this Aug 21, 2022
@@ -77,6 +77,19 @@ static GaussianMixtureFactor::Sum &addGaussian(
}

/* ************************************************************************ */
/* Function to eliminate variables **under the following assumptions**:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment should be adapted here (it's good for the EliminateHybrid function)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh okay. This was the function where we had the long discussion on the other PR so I figured this is where you wanted it.
Should we add another comment for sumFrontals?

deferredFactors.push_back(
boost::dynamic_pointer_cast<HybridGaussianFactor>(f)->inner());
// Check if f is HybridConditional or HybridGaussianFactor.
if (auto hc = boost::dynamic_pointer_cast<HybridConditional>(f)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should just work without explicit casting as HybridConditional is a subclass of HybridGaussianConditional

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I don't remember why I did this. Guess I was debugging something.

@varunagrawal
Copy link
Collaborator Author

Looks like we have to upgrade to Ubuntu 20.04.

actions/runner-images#6002

Base automatically changed from feature/nonlinear-incremental to feature/nonlinear-hybrid August 22, 2022 18:13
@varunagrawal varunagrawal requested a review from ProfFan August 22, 2022 18:13
@varunagrawal varunagrawal merged commit 84456f4 into feature/nonlinear-hybrid Aug 22, 2022
@varunagrawal varunagrawal deleted the hybrid-incremental branch August 22, 2022 18:20
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.

2 participants