Nonlinear Hybrid #1263
Nonlinear Hybrid #1263
Conversation
…ass of HybridFactor
varunagrawal
force-pushed
the
feature/nonlinear-hybrid
branch
from
588f56e
to
a3eacaa
Compare
| @@ -98,6 +98,12 @@ GaussianMixtureFactor::Sum sumFrontals( | |||
| } else if (f->isContinuous()) { | |||
| deferredFactors.push_back( | |||
| boost::dynamic_pointer_cast<HybridGaussianFactor>(f)->inner()); | |||
|
|
|||
| } else if (f->isDiscrete()) { | |||
There was a problem hiding this comment.
Is this change necessary? I remember at this step there should be no pure discrete factors.
There was a problem hiding this comment.
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.
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.
I think because this is a GaussianMixtureFactor, there should not be any discrete factor added to it
There was a problem hiding this comment.
This is a Hybrid Factor Graph method so a discrete factor can be present.
There was a problem hiding this comment.
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.
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.
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
Function to eliminate variables **under the following assumptions**:
1. When the ordering is fully continuous, and the graph only contains continuous and hybrid factors
2. When the ordering is fully discrete, and the graph only contains discrete factors
Any usage outside of this is incorrect.
\warning This function is not meant to be used with arbitrary hybrid factor graphs. For example, if there exists continuous parents, and one tries to eliminate a discrete variable (as specified in the ordering), the result will be INCORRECT and there will be NO error raised.
There was a problem hiding this comment.
How does that sound to you?
There was a problem hiding this comment.
Sounds good to me.
|
Folks, there are PRs before this one that need to be reviewed and merged first to make your lives easier. :) |
| nlf->linearize(continuousValues)); | ||
| linearFG.push_back(hgf); | ||
| } else { | ||
| linearFG.push_back(factor); |
There was a problem hiding this comment.
(ignore if not right) So we do allow pushing linear factors to the Hybrid NLFG? Because I remember that there is a LinearContainerFactor, not sure if we want to do the same here.
There was a problem hiding this comment.
Yes. This is something we can look at again once we update the type hierarchy for hybrid.
| // IncrementalHybrid incrementalHybrid; | ||
| // HybridGaussianFactorGraph graph1; | ||
|
|
||
| // // Add the 3 DC factors, x1-x2, x2-x3, x3-x4 |
There was a problem hiding this comment.
These are uncommented in the next PR, right?
| factors.push_back(self.linearizedFactorGraph[1]); // involves m1 | ||
| factors.push_back(self.linearizedFactorGraph[2]); // involves m2 | ||
|
|
||
| // TODO(Varun) remove this block since sum is no longer exposed. |
There was a problem hiding this comment.
Yeah I kept this since I wanted to highlight this change in the PR
Add Nonlinear Hybrid Factor Graph machinery.
I moved all the tests for switching systems from our previous scheme to our current scheme and all of them pass!