TableFactor and TableDistribution #1953
Conversation
…ableConditional since it provides a clean abstraction
varunagrawal
changed the base branch from
hybrid-timing
to
hybrid-custom-discrete
varunagrawal
changed the title
| @@ -450,11 +450,18 @@ TEST(HybridBayesNet, UpdateDiscreteConditionals) { | |||
|
|
|||
| DiscreteConditional joint; | |||
| for (auto&& conditional : posterior->discreteMarginal()) { | |||
| joint = joint * (*conditional); | |||
| // The last discrete conditional may be a TableDistribution | |||
There was a problem hiding this comment.
I think this code should still work as is.
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| #include <gtsam/discrete/DiscreteFactorGraph.h> | |||
| #include <gtsam/discrete/DiscreteKey.h> | |||
| #include <gtsam/discrete/TableDistribution.h> | |||
There was a problem hiding this comment.
Why? Move to cpp to remove header bloat
| @@ -41,6 +42,22 @@ bool HybridBayesTree::equals(const This& other, double tol) const { | |||
| return Base::equals(other, tol); | |||
| } | |||
|
|
|||
| /* ************************************************************************* */ | |||
| DiscreteValues HybridBayesTree::discreteMaxProduct( | |||
There was a problem hiding this comment.
This should be in DiscreteFactorGraph?
There was a problem hiding this comment.
This is special behavior leveraging the TableFactor. I don't think we should mess with DiscreteFactorGraph wrt this.
gtsam/hybrid/HybridBayesNet.cpp
Outdated
| @@ -52,15 +52,24 @@ HybridBayesNet HybridBayesNet::prune(size_t maxNrLeaves) const { | |||
| // Multiply into one big conditional. NOTE: possibly quite expensive. | |||
| DiscreteConditional joint; | |||
| for (auto &&conditional : marginal) { | |||
| joint = joint * (*conditional); | |||
| // The last discrete conditional may be a TableDistribution | |||
There was a problem hiding this comment.
Why does code no longer work as is?
There was a problem hiding this comment.
Must have been from before we refactored discrete multiplication.
There was a problem hiding this comment.
Think about how it can still work this way, please? Seems it should be possible. The new code is too complex
There was a problem hiding this comment.
So this is not working as is because we enabled multiplication for DiscreteFactor::shared_ptr so the result is a DiscreteFactor::shared_ptr which has no concept of frontal variables.
We can either convert this to DiscreteFactors and then convert to a DiscreteConditional at the end, or use a TableDistribution as the type for joint.
There was a problem hiding this comment.
Just seeing this comment. This will work with the fix we discussed, right? Noticed the comments is older than that...
There was a problem hiding this comment.
I am starting to wonder if this has been a distraction. The previous HybridBayesNet::TableProduct method worked great for us.
|
NOTE: I had to override |
|
I’ll let you merge |
Use the
TableFactorfor discrete elimination and use a newTableDistributionwhich maintains an underlying representation of the distribution using the sameSparseVectormechanism.