-
Notifications
You must be signed in to change notification settings - Fork 795
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
Nonlinear Hybrid #1263
Nonlinear Hybrid #1263
Changes from all commits
2927d92
78ea90b
e91a354
9cbd2ef
01b9a65
fe0d666
cdd030b
08fab8a
9279bd6
3274cb1
6c36b2c
85f4b48
53e8c32
7e18277
119679a
3212dde
0c16799
e711a62
3780b8c
8ddc2ea
7a55341
43c28e7
987448f
8471c97
8907922
16124f3
ee124c3
b39c231
0f732d7
6670779
92a5868
db56909
5965d8f
f5e046f
51d2f07
a3eacaa
588f56e
60c88e3
4ea897c
fbceda3
0e4db30
2a974a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* ---------------------------------------------------------------------------- | ||
|
||
* GTSAM Copyright 2010, Georgia Tech Research Corporation, | ||
* Atlanta, Georgia 30332-0415 | ||
* All Rights Reserved | ||
* Authors: Frank Dellaert, et al. (see THANKS for the full author list) | ||
|
||
* See LICENSE for the license information | ||
|
||
* -------------------------------------------------------------------------- */ | ||
|
||
/** | ||
* @file HybridNonlinearFactor.cpp | ||
* @date May 28, 2022 | ||
* @author Varun Agrawal | ||
*/ | ||
|
||
#include <gtsam/hybrid/HybridNonlinearFactor.h> | ||
|
||
#include <boost/make_shared.hpp> | ||
|
||
namespace gtsam { | ||
|
||
/* ************************************************************************* */ | ||
HybridNonlinearFactor::HybridNonlinearFactor( | ||
const NonlinearFactor::shared_ptr &other) | ||
: Base(other->keys()), inner_(other) {} | ||
|
||
/* ************************************************************************* */ | ||
bool HybridNonlinearFactor::equals(const HybridFactor &lf, double tol) const { | ||
return Base::equals(lf, tol); | ||
} | ||
|
||
/* ************************************************************************* */ | ||
void HybridNonlinearFactor::print(const std::string &s, | ||
const KeyFormatter &formatter) const { | ||
HybridFactor::print(s, formatter); | ||
inner_->print("\n", formatter); | ||
}; | ||
|
||
} // namespace gtsam |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* ---------------------------------------------------------------------------- | ||
|
||
* GTSAM Copyright 2010, Georgia Tech Research Corporation, | ||
* Atlanta, Georgia 30332-0415 | ||
* All Rights Reserved | ||
* Authors: Frank Dellaert, et al. (see THANKS for the full author list) | ||
|
||
* See LICENSE for the license information | ||
|
||
* -------------------------------------------------------------------------- */ | ||
|
||
/** | ||
* @file HybridNonlinearFactor.h | ||
* @date May 28, 2022 | ||
* @author Varun Agrawal | ||
*/ | ||
|
||
#pragma once | ||
|
||
#include <gtsam/hybrid/HybridFactor.h> | ||
#include <gtsam/hybrid/HybridGaussianFactor.h> | ||
#include <gtsam/nonlinear/NonlinearFactor.h> | ||
|
||
namespace gtsam { | ||
|
||
/** | ||
* A HybridNonlinearFactor is a layer over NonlinearFactor so that we do not | ||
* have a diamond inheritance. | ||
*/ | ||
class HybridNonlinearFactor : public HybridFactor { | ||
private: | ||
NonlinearFactor::shared_ptr inner_; | ||
|
||
public: | ||
using Base = HybridFactor; | ||
using This = HybridNonlinearFactor; | ||
using shared_ptr = boost::shared_ptr<This>; | ||
|
||
// Explicit conversion from a shared ptr of NonlinearFactor | ||
explicit HybridNonlinearFactor(const NonlinearFactor::shared_ptr &other); | ||
|
||
/// @name Testable | ||
/// @{ | ||
|
||
/// Check equality. | ||
virtual bool equals(const HybridFactor &lf, double tol) const override; | ||
|
||
/// GTSAM print utility. | ||
void print( | ||
const std::string &s = "HybridNonlinearFactor\n", | ||
const KeyFormatter &formatter = DefaultKeyFormatter) const override; | ||
|
||
/// @} | ||
|
||
NonlinearFactor::shared_ptr inner() const { return inner_; } | ||
|
||
/// Linearize to a HybridGaussianFactor at the linearization point `c`. | ||
boost::shared_ptr<HybridGaussianFactor> linearize(const Values &c) const { | ||
return boost::make_shared<HybridGaussianFactor>(inner_->linearize(c)); | ||
} | ||
}; | ||
} // namespace gtsam |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/* ---------------------------------------------------------------------------- | ||
|
||
* GTSAM Copyright 2010, Georgia Tech Research Corporation, | ||
* Atlanta, Georgia 30332-0415 | ||
* All Rights Reserved | ||
* Authors: Frank Dellaert, et al. (see THANKS for the full author list) | ||
|
||
* See LICENSE for the license information | ||
|
||
* -------------------------------------------------------------------------- */ | ||
|
||
/** | ||
* @file HybridNonlinearFactorGraph.cpp | ||
* @brief Nonlinear hybrid factor graph that uses type erasure | ||
* @author Varun Agrawal | ||
* @date May 28, 2022 | ||
*/ | ||
|
||
#include <gtsam/hybrid/HybridNonlinearFactorGraph.h> | ||
|
||
namespace gtsam { | ||
|
||
/* ************************************************************************* */ | ||
void HybridNonlinearFactorGraph::add( | ||
boost::shared_ptr<NonlinearFactor> factor) { | ||
FactorGraph::add(boost::make_shared<HybridNonlinearFactor>(factor)); | ||
} | ||
|
||
/* ************************************************************************* */ | ||
void HybridNonlinearFactorGraph::print(const std::string& s, | ||
const KeyFormatter& keyFormatter) const { | ||
// Base::print(str, keyFormatter); | ||
std::cout << (s.empty() ? "" : s + " ") << std::endl; | ||
std::cout << "size: " << size() << std::endl; | ||
for (size_t i = 0; i < factors_.size(); i++) { | ||
std::stringstream ss; | ||
ss << "factor " << i << ": "; | ||
if (factors_[i]) { | ||
factors_[i]->print(ss.str(), keyFormatter); | ||
std::cout << std::endl; | ||
} | ||
} | ||
} | ||
|
||
/* ************************************************************************* */ | ||
HybridGaussianFactorGraph HybridNonlinearFactorGraph::linearize( | ||
const Values& continuousValues) const { | ||
// create an empty linear FG | ||
HybridGaussianFactorGraph linearFG; | ||
|
||
linearFG.reserve(size()); | ||
|
||
// linearize all hybrid factors | ||
for (auto&& factor : factors_) { | ||
// First check if it is a valid factor | ||
if (factor) { | ||
// Check if the factor is a hybrid factor. | ||
// It can be either a nonlinear MixtureFactor or a linear | ||
// GaussianMixtureFactor. | ||
if (factor->isHybrid()) { | ||
// Check if it is a nonlinear mixture factor | ||
if (auto nlmf = boost::dynamic_pointer_cast<MixtureFactor>(factor)) { | ||
linearFG.push_back(nlmf->linearize(continuousValues)); | ||
} else { | ||
linearFG.push_back(factor); | ||
} | ||
|
||
// Now check if the factor is a continuous only factor. | ||
} else if (factor->isContinuous()) { | ||
// In this case, we check if factor->inner() is nonlinear since | ||
// HybridFactors wrap over continuous factors. | ||
auto nlhf = boost::dynamic_pointer_cast<HybridNonlinearFactor>(factor); | ||
if (auto nlf = | ||
boost::dynamic_pointer_cast<NonlinearFactor>(nlhf->inner())) { | ||
auto hgf = boost::make_shared<HybridGaussianFactor>( | ||
nlf->linearize(continuousValues)); | ||
linearFG.push_back(hgf); | ||
} else { | ||
linearFG.push_back(factor); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (ignore if not right) So we do allow pushing linear factors to the Hybrid NLFG? Because I remember that there is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. This is something we can look at again once we update the type hierarchy for hybrid. |
||
} | ||
// Finally if nothing else, we are discrete-only which doesn't need | ||
// lineariztion. | ||
} else { | ||
linearFG.push_back(factor); | ||
} | ||
|
||
} else { | ||
linearFG.push_back(GaussianFactor::shared_ptr()); | ||
} | ||
} | ||
return linearFG; | ||
} | ||
|
||
} // namespace gtsam |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change necessary? I remember at this step there should be no pure discrete factors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there was one test that was failing without this. You can probably comment it out and find out which one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember now. If we have discrete only factors it goes to the next condition and errors out since it is not an orphan. That shouldn't happen right? If we only have discrete factors left, we should just be returning a discrete decision tree and not erroring out.
@ProfFan can you please explain what was your rationale here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think because this is a GaussianMixtureFactor, there should not be any discrete factor added to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Hybrid Factor Graph method so a discrete factor can be present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not opposing to adding this branching. I am a bit worried that people could start to use this function without thinking about the preconditions when this operation is valid. So I prefer to keep this function as-is, or add some comments in the function Doxygen as a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay now this makes sense. In that case, you could have mentioned this before rather than simply saying this is wrong. I am not aware of your mental model and there aren't any comments about the same.
I believe we should still test
EliminateHybrid
individually. Letting users know what the assumptions are is the right way here, but we should also ensure we handle these kind of edge cases so it makes using the API easier. Can you please comment what would you like me to add as a note in the docstring and I'll add that to this PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to talk about the actual situation that will happen when this is called on an arbitrary factor graph. What I can think of is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that sound to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.