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

HybridNonlinearFactor Relinearization #1831

Merged
merged 30 commits into from
Sep 18, 2024
Merged

Conversation

varunagrawal
Copy link
Collaborator

HybridNonlinearFactor and HybridGaussianFactor now take a DecisionTree of factor and scalar pairs. This allows us to specify arbitrary values which will be used within the definition of the factors, allowing for relinearization.

@varunagrawal
Copy link
Collaborator Author

@dellaert take a look while I finish adding some tests for HybridNonlinearFactorGraph to verify this works correctly. :)

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.

Made some comments regarding a vector-based constructor, which I think is problematic, unless this is a convenience constructor for a single mode? If there are multiple discrete keys, specifying a vector is always going to lead to bugs.

gtsam/hybrid/HybridGaussianConditional.cpp Outdated Show resolved Hide resolved
gtsam/hybrid/HybridGaussianFactor.h Show resolved Hide resolved
gtsam/hybrid/HybridGaussianFactor.h Outdated Show resolved Hide resolved
gtsam/hybrid/tests/Switching.h Outdated Show resolved Hide resolved
gtsam/hybrid/tests/Switching.h Outdated Show resolved Hide resolved
gtsam/hybrid/HybridNonlinearFactor.h Show resolved Hide resolved
gtsam/hybrid/HybridNonlinearFactor.h Outdated Show resolved Hide resolved
gtsam/hybrid/HybridNonlinearFactor.h Outdated Show resolved Hide resolved
gtsam/hybrid/tests/testHybridBayesNet.cpp Outdated Show resolved Hide resolved
@varunagrawal varunagrawal marked this pull request as ready for review September 15, 2024 13:18
for (auto &&f : motion_models) {
components.push_back(std::dynamic_pointer_cast<NonlinearFactor>(f));
components.push_back(
{std::dynamic_pointer_cast<NonlinearFactor>(f), 0.0});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dellaert here's one reason to keep the vector based constructor. Allows us to define the tree incrementally like this.

@varunagrawal
Copy link
Collaborator Author

@dellaert based on our discussion, I have updated the API and tests.

@@ -107,25 +107,13 @@ class GTSAM_EXPORT HybridGaussianConditional
const Conditionals &conditionals);

/**
* @brief Make a Gaussian Mixture from a list of Gaussian conditionals
* @brief Make a Gaussian Mixture from a vector of Gaussian conditionals.
* The DecisionTree-based constructor is preferred over this one.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kept this constructor for the python wrapper, but added this note.

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 think this looks good, but I would have the single discrete key constructor as we discussed?

Z_1x1, 0.5),
GaussianConditional::sharedMeanAndStddev(Z(i), I_1x1, X(0),
Z_1x1, 3)});
HybridGaussianConditional::Conditionals(modes, conditionals));
Copy link
Member

Choose a reason for hiding this comment

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

So, this is the case of a single key, so here you can use a vector, as we discussed. You would then specify a single DiscreteKey, not a set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah sorry about that, I had already changed to the DecisionTree constructor before we discussed the single key option. Updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI this is for HybridGaussianConditional which can have multiple discrete parents. I've kept this for multiple discrete keys mainly for the wrapper.
Should I still go ahead and update this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going ahead and doing this in the interest of time. If a use case comes up where we need multiple discrete parents + the vector of conditionals, we can re-evaluate then. :)

KeyVector{X(1)}, KeyVector{}, DiscreteKeys{Asia},
std::vector{conditional0, conditional1});
KeyVector{X(1)}, KeyVector{}, discreteParents,
HybridGaussianConditional::Conditionals(
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

KeyVector{X(1)}, KeyVector{}, DiscreteKeys{Asia},
std::vector{conditional0, conditional1});
KeyVector{X(1)}, KeyVector{}, discreteParents,
HybridGaussianConditional::Conditionals(
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

DiscreteKeys discreteKeys{DiscreteKey(M(0), 2)};
HybridNonlinearFactor::Factors factors(
discreteKeys, {{zero_motion, 0.0}, {one_motion, 0.0}});
nfg.emplace_shared<HybridNonlinearFactor>(KeyVector{X(0), X(1)}, discreteKeys,
Copy link
Member

Choose a reason for hiding this comment

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

And here as well…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

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!

@varunagrawal varunagrawal merged commit f7b5f3c into develop Sep 18, 2024
33 checks passed
@varunagrawal varunagrawal deleted the hybrid-error-scalars branch September 18, 2024 20:17
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