-
Notifications
You must be signed in to change notification settings - Fork 804
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
Common errorTree
method and its use in HybridGaussianFactorGraph
#1837
Changes from all commits
cd3c590
9c3d7b0
8231de2
9cbc754
c71f033
d145872
939fdcc
4c82248
d81cd82
821b22f
9f9032f
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 |
---|---|---|
|
@@ -136,6 +136,10 @@ class GTSAM_EXPORT HybridFactor : public Factor { | |
/// Return only the continuous keys for this factor. | ||
const KeyVector &continuousKeys() const { return continuousKeys_; } | ||
|
||
/// Virtual class to compute tree of linear errors. | ||
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. linear? 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. This linear because it only accepts |
||
virtual AlgebraicDecisionTree<Key> errorTree( | ||
const VectorValues &values) const = 0; | ||
|
||
/// @} | ||
|
||
private: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,13 @@ class GTSAM_EXPORT HybridNonlinearFactor : public HybridFactor { | |
/// Decision tree of Gaussian factors indexed by discrete keys. | ||
Factors factors_; | ||
|
||
/// HybridFactor method implementation. Should not be used. | ||
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. Hmmm. So then why does it exist in the base class. red flag. Should maybe only exist in Gaussian branch. 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. This is primarily because of HybridConditional. The class hierarchy is
HybridConditional acts as a common container class for GaussianConditional, DiscreteConditional and HybridGaussianConditional. By defining errorTree in HybridFactor, I can just check for HybridFactor. Otherwise, I'll have to check for HybridConditional and HybridGaussianFactor separately which I felt defeated the purpose of this base class method. 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. So, this is a different hierarchy than the non-hybrid case then:
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. Sorry there was a rendering error in my previous comment. 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. What would break if you only have errorTree in HybridGaussianFactor? It would be available in HGC. All done? 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. Why is a check needed? We would never call errorTree on HybridConditional, as it only makes sense for Gaussian variants, right? 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. A HybridConditional can have a GaussianConditional or a HybridGaussianConditional as its underlying conditional. Remember that HybridConditional is a container based on type erasure and not a base class for HybridGaussianConditional. 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. Right. But then why would we need an errorTree function for HybridConditional. If it can be either, then a method providing VectorValues does not make sense to me. Where would that be used? Is it used??? 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. It is used in an incremental sense, when we add the conditionals from a HybridBayesNet to the HybridGaussianFactorGraph, the factor graph now has The alternative could be to unwrap the conditionals from the I am open to suggestions about this since this was nontrivial for me. I think checking for 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. Oh and another case: if we go from a |
||
AlgebraicDecisionTree<Key> errorTree( | ||
const VectorValues& continuousValues) const override { | ||
throw std::runtime_error( | ||
"HybridNonlinearFactor::error does not take VectorValues."); | ||
} | ||
|
||
public: | ||
HybridNonlinearFactor() = default; | ||
|
||
|
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.
Why not in HybridFactor?
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.
HybridFactor
doesn't have theasGaussian
,asHybrid
andasDiscrete
methods.