Common errorTree method and its use in HybridGaussianFactorGraph
#1837
Conversation
| @@ -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.
This linear because it only accepts VectorValues right? This is also why its definition in HybridNonlinearFactor throws a runtime error.
| @@ -129,6 +129,22 @@ double HybridConditional::error(const HybridValues &values) const { | |||
| "HybridConditional::error: conditional type not handled"); | |||
| } | |||
|
|
|||
| /* ************************************************************************ */ | |||
| AlgebraicDecisionTree<Key> HybridConditional::errorTree( | |||
There was a problem hiding this comment.
HybridFactor doesn't have the asGaussian, asHybrid and asDiscrete methods.
| @@ -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.
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.
This is primarily because of HybridConditional.
The class hierarchy is
- HybridFactor
- HybridConditional
- HybridGaussianFactor
- HybridGaussianConditional
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.
So, this is a different hierarchy than the non-hybrid case then:
class GTSAM_EXPORT GaussianConditional :
public JacobianFactor,
public Conditional<JacobianFactor, GaussianConditional>
There was a problem hiding this comment.
Sorry there was a rendering error in my previous comment.
It is the same hierarchy as GaussianConditional since HybridGaussianConditional inherits from HybridGaussianFactor, however HybridConditional complicates things (or at least makes it messier).
There was a problem hiding this comment.
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.
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.
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.
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.
It is used in an incremental sense, when we add the conditionals from a HybridBayesNet to the HybridGaussianFactorGraph, the factor graph now has HybridGaussianFactor (and potential HybridGaussianConditionals) and HybridConditionals as well.
The alternative could be to unwrap the conditionals from the HybridConditional before adding it to the graph.
I am open to suggestions about this since this was nontrivial for me. I think checking for HybridGaussianFactor and HybridConditional separately makes sense.
There was a problem hiding this comment.
Oh and another case: if we go from a HybridBayesNet to a HybridGaussianFactorGraph, then all the factors are added as HybridConditionals which are represented as HybridFactors. This causes DiscreteFactors to be considered in the errorTree computations as well. :(
Yeah I think we can come up with a better class inheritance hierarchy, which is transparent to the user, later once the API is stable. I have ideas about this! |
Add
errorTreeas a virtual method inHybridFactor, and implement it in child classes.This allows us to make the
HybridGaussianFactorGraph::errorTreemethod very clean.Also added the
HybridNonlinearFactorGraph::error(const HybridValues&)method and corresponding test.