Simpler HybridGaussianFoo constructors #1848
Conversation
dellaert
commented
Sep 26, 2024
dellaert
commented
- Remove superfluous arguments
- Add helper structs and private constructor to ensure proper initialization
- Added input checking on factors and conditional keys but removed it since
- expensive
- it was not checked before
- simplified all call sites -> no code is the best code
|
Updating to also fix HybridNonlinearFactor |
| const DiscreteKey &discreteKey, | ||
| const std::vector<GaussianFactor::shared_ptr> &factors) | ||
| : discreteKeys({discreteKey}) { | ||
| // Extract continuous keys from the first non-null factor |
There was a problem hiding this comment.
I'll do a full review in a bit, but I am wondering about the extra compute happening here when we could have just given the keys directly.
Currently I am struggling with the hybrid state estimator being far too slow and having too many hypotheses. Pruning is working but the blow up is still immense, hence my concerns here.
There was a problem hiding this comment.
Ironically, it's a bit faster.
There was a problem hiding this comment.
Esp, GaussianConditional, which did two tree traversals, now only does one.
That's fine, I'm moving some more stuff to cpp files now rather than cluttering the headers. |
OK, done, and cleaned up some missing python test changes. |
| const ConstructorHelper &helper) | ||
| : BaseFactor(discreteParents, helper.pairs), | ||
| BaseConditional(helper.frontals.size()), | ||
| conditionals_(conditionals), |
There was a problem hiding this comment.
These things are here twice! Once in BaseFactor, and once in conditionals_.
There was a problem hiding this comment.
Yeah I thought about that too. There was a reason I couldn't get rid of conditionals_, which I think was to do with negLogConstant. I'll think about this again so we can remove it.
| value = c->negLogConstant(); | ||
| negLogConstant = std::min(negLogConstant, value); | ||
| } | ||
| return {std::dynamic_pointer_cast<GaussianFactor>(c), value}; |
There was a problem hiding this comment.
Because we store the value, we should never have to call negLogConstant in this file again!
And that’s why we currently need to keep conditionals :-( at least until the hiding is removed.
|
@varunagrawal whats up with windows CI? |
|
I think the boost download link is invalid. It's affecting multiple PRs. |
| const ConstructorHelper &helper) | ||
| : BaseFactor(discreteParents, helper.pairs), | ||
| BaseConditional(helper.frontals.size()), | ||
| conditionals_(conditionals), |
There was a problem hiding this comment.
Yeah I thought about that too. There was a reason I couldn't get rid of conditionals_, which I think was to do with negLogConstant. I'll think about this again so we can remove it.
| const std::vector<GaussianFactorValuePair> &factorPairs) | ||
| : HybridGaussianFactor(ConstructorHelper(discreteKey, factorPairs)) {} | ||
|
|
||
| HybridGaussianFactor::HybridGaussianFactor(const DiscreteKeys &discreteKeys, |
There was a problem hiding this comment.
All three of the above constructors need the divider
/* *******************************************************************************/
There was a problem hiding this comment.
This is more for me when I add it.
