Re-linearization #2017
Re-linearization #2017
Conversation
dellaert
commented
Feb 2, 2025
- extensive implementation of restrict and re-linearization
- some are not implemented, which is ok for now.
|
Yay, @varunagrawal , tests finally pass. Had to fix some stragglers. |
| @@ -153,7 +153,8 @@ class GTSAM_EXPORT HybridConditional | |||
| * @return HybridGaussianConditional::shared_ptr otherwise | |||
| */ | |||
| HybridGaussianConditional::shared_ptr asHybrid() const { | |||
| return std::dynamic_pointer_cast<HybridGaussianConditional>(inner_); | |||
| if (!isHybrid()) return nullptr; | |||
| return std::static_pointer_cast<HybridGaussianConditional>(inner_); | |||
There was a problem hiding this comment.
Wow we've been doing it wrong for a while now.
There was a problem hiding this comment.
Actually do we want this? If we call asHybrid on a non-hybrid factor, it should error out. I recommend updating the (!isHybrid()) check to throw an exception perhaps?
There was a problem hiding this comment.
No, disagree. I did not change the semantics here, just the performance. I like these semantics.
| @@ -162,7 +163,8 @@ class GTSAM_EXPORT HybridConditional | |||
| * @return GaussianConditional::shared_ptr otherwise | |||
| */ | |||
| GaussianConditional::shared_ptr asGaussian() const { | |||
| return std::dynamic_pointer_cast<GaussianConditional>(inner_); | |||
| if (!isContinuous()) return nullptr; | |||
There was a problem hiding this comment.
Preferably throw exception instead of returning a nullptr since this can be a silent error/failure.
| @@ -172,7 +174,8 @@ class GTSAM_EXPORT HybridConditional | |||
| */ | |||
| template <typename T = DiscreteConditional> | |||
| typename T::shared_ptr asDiscrete() const { | |||
| return std::dynamic_pointer_cast<T>(inner_); | |||
| if (!isDiscrete()) return nullptr; | |||
There was a problem hiding this comment.
Preferably throw exception instead of returning a nullptr since this can be a silent error/failure.
There was a problem hiding this comment.
No, @varunagrawal , this is used to find out whether it is of this type. I don’t think we should mess with that semantics
| } else if (auto df = dynamic_pointer_cast<DiscreteFactor>(f)) { | ||
| result.push_back(df->restrict(discreteValues)); | ||
| } else { | ||
| result.push_back(f); // Everything else is just added as is |
There was a problem hiding this comment.
Add a TODO here. Potentially lots of performance left on the table here, particularly in HybridNonlinearISAM where we keep track of the nonlinear factors and relinearize.
There was a problem hiding this comment.
Maybe. These are shared pointers so not costly here
| /* ************************************************************************ */ | ||
| DiscreteFactor::shared_ptr DecisionTreeFactor::restrict( | ||
| const DiscreteValues& assignment) const { | ||
| throw std::runtime_error("DecisionTreeFactor::restrict not implemented"); |
There was a problem hiding this comment.
This should just be Base::restrict and convert to a factor, right?
There was a problem hiding this comment.
Hmm, is already a factor, so it should restrict using the fact that it is a decision tree, and then return a restricted decision tree. Base::restrict can’t do that.
|
Merging so I can merge into my own PR. |