From b91c470b6929bffca39bfbd36c39c3795183c79e Mon Sep 17 00:00:00 2001 From: Varun Agrawal Date: Fri, 13 Dec 2024 08:07:10 -0500 Subject: [PATCH 1/4] test exposing incorrect conversion --- gtsam/discrete/tests/testTableFactor.cpp | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/gtsam/discrete/tests/testTableFactor.cpp b/gtsam/discrete/tests/testTableFactor.cpp index 212067cb3..7df6da83e 100644 --- a/gtsam/discrete/tests/testTableFactor.cpp +++ b/gtsam/discrete/tests/testTableFactor.cpp @@ -147,6 +147,36 @@ TEST(TableFactor, constructors) { EXPECT(assert_inequal(f5_with_wrong_keys, f5, 1e-9)); } +/* ************************************************************************* */ +// Check conversion from DecisionTreeFactor. +TEST(TableFactor, Conversion) { + /* This is the DecisionTree we are using + Choice(m2) + 0 Choice(m1) + 0 0 Leaf 0 + 0 1 Choice(m0) + 0 1 0 Leaf 0 + 0 1 1 Leaf 0.14649446 // 3 + 1 Choice(m1) + 1 0 Choice(m0) + 1 0 0 Leaf 0 + 1 0 1 Leaf 0.14648756 // 5 + 1 1 Choice(m0) + 1 1 0 Leaf 0.14649446 // 6 + 1 1 1 Leaf 0.23918345 // 7 + */ + DiscreteKeys dkeys = {{0, 2}, {1, 2}, {2, 2}}; + DecisionTreeFactor dtf( + dkeys, std::vector{0, 0, 0, 0.14649446, 0, 0.14648756, 0.14649446, + 0.23918345}); + + // dtf.print(); + TableFactor tf(dtf.discreteKeys(), dtf); + // tf.print(); + // tf.toDecisionTreeFactor().print(); + EXPECT(assert_equal(dtf, tf.toDecisionTreeFactor())); +} + /* ************************************************************************* */ // Check multiplication between two TableFactors. TEST(TableFactor, multiplication) { From a8e24efdeca680f391098979ad2402dd3918153f Mon Sep 17 00:00:00 2001 From: Varun Agrawal Date: Fri, 13 Dec 2024 09:34:01 -0500 Subject: [PATCH 2/4] update ComputeLeafOrdering to give a correct vector of values --- gtsam/discrete/TableFactor.cpp | 72 +++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/gtsam/discrete/TableFactor.cpp b/gtsam/discrete/TableFactor.cpp index 8e185eb3b..a2d68853e 100644 --- a/gtsam/discrete/TableFactor.cpp +++ b/gtsam/discrete/TableFactor.cpp @@ -64,27 +64,69 @@ TableFactor::TableFactor(const DiscreteKeys& dkeys, /** * @brief Compute the correct ordering of the leaves in the decision tree. * - * This is done by first taking all the values which have modulo 0 value with - * the cardinality of the innermost key `n`, and we go up to modulo n. - * * @param dt The DecisionTree - * @return std::vector + * @return Eigen::SparseVector */ -std::vector ComputeLeafOrdering(const DiscreteKeys& dkeys, - const DecisionTreeFactor& dt) { - std::vector probs = dt.probabilities(); - std::vector ordered; +static Eigen::SparseVector ComputeLeafOrdering( + const DiscreteKeys& dkeys, const DecisionTreeFactor& dt) { + // SparseVector needs to know the maximum possible index, + // so we compute the product of cardinalities. + size_t prod_cardinality = 1; + for (auto&& [_, c] : dt.cardinalities()) { + prod_cardinality *= c; + } + Eigen::SparseVector sparse_table(prod_cardinality); + size_t nrValues = 0; + dt.visit([&nrValues](double x) { + if (x > 0) nrValues += 1; + }); + sparse_table.reserve(nrValues); - size_t n = dkeys[0].second; + std::set allKeys(dt.keys().begin(), dt.keys().end()); - for (size_t k = 0; k < n; ++k) { - for (size_t idx = 0; idx < probs.size(); ++idx) { - if (idx % n == k) { - ordered.push_back(probs[idx]); + auto op = [&](const Assignment& assignment, double p) { + if (p > 0) { + // Get all the keys involved in this assignment + std::set assignment_keys; + for (auto&& [k, _] : assignment) { + assignment_keys.insert(k); + } + + // Find the keys missing in the assignment + std::vector diff; + std::set_difference(allKeys.begin(), allKeys.end(), + assignment_keys.begin(), assignment_keys.end(), + std::back_inserter(diff)); + + // Generate all assignments using the missing keys + DiscreteKeys extras; + for (auto&& key : diff) { + extras.push_back({key, dt.cardinality(key)}); + } + auto&& extra_assignments = DiscreteValues::CartesianProduct(extras); + + for (auto&& extra : extra_assignments) { + // Create new assignment using the extra assignment + DiscreteValues updated_assignment(assignment); + updated_assignment.insert(extra); + + // Generate index and add to the sparse vector. + Eigen::Index idx = 0; + size_t prev_cardinality = 1; + // We go in reverse since a DecisionTree has the highest label first + for (auto&& it = updated_assignment.rbegin(); + it != updated_assignment.rend(); it++) { + idx += prev_cardinality * it->second; + prev_cardinality *= dt.cardinality(it->first); + } + sparse_table.coeffRef(idx) = p; } } - } - return ordered; + }; + + dt.visitWith(op); + + return sparse_table; } /* ************************************************************************ */ From 7d389a53007af3811a0c8195d368e8fcd3825c3f Mon Sep 17 00:00:00 2001 From: Varun Agrawal Date: Fri, 13 Dec 2024 09:46:55 -0500 Subject: [PATCH 3/4] clean up test --- gtsam/discrete/tests/testTableFactor.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gtsam/discrete/tests/testTableFactor.cpp b/gtsam/discrete/tests/testTableFactor.cpp index 7df6da83e..a455faaaa 100644 --- a/gtsam/discrete/tests/testTableFactor.cpp +++ b/gtsam/discrete/tests/testTableFactor.cpp @@ -170,10 +170,8 @@ TEST(TableFactor, Conversion) { dkeys, std::vector{0, 0, 0, 0.14649446, 0, 0.14648756, 0.14649446, 0.23918345}); - // dtf.print(); TableFactor tf(dtf.discreteKeys(), dtf); - // tf.print(); - // tf.toDecisionTreeFactor().print(); + EXPECT(assert_equal(dtf, tf.toDecisionTreeFactor())); } From 9830981351fd5e5b714c6a5dc58b6e67ba8ec7cc Mon Sep 17 00:00:00 2001 From: Varun Agrawal Date: Fri, 13 Dec 2024 13:58:19 -0500 Subject: [PATCH 4/4] address review comments --- gtsam/discrete/TableFactor.cpp | 61 ++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/gtsam/discrete/TableFactor.cpp b/gtsam/discrete/TableFactor.cpp index a2d68853e..de1e1f867 100644 --- a/gtsam/discrete/TableFactor.cpp +++ b/gtsam/discrete/TableFactor.cpp @@ -62,40 +62,55 @@ TableFactor::TableFactor(const DiscreteKeys& dkeys, : TableFactor(dkeys, DecisionTreeFactor(dkeys, dtree)) {} /** - * @brief Compute the correct ordering of the leaves in the decision tree. + * @brief Compute the indexing of the leaves in the decision tree based on the + * assignment and add the (index, leaf) pair to a SparseVector. + * + * We visit each leaf in the tree, and using the cardinalities of the keys, + * compute the correct index to add the leaf to a SparseVector which + * is then used to create the TableFactor. * * @param dt The DecisionTree * @return Eigen::SparseVector */ -static Eigen::SparseVector ComputeLeafOrdering( +static Eigen::SparseVector ComputeSparseTable( const DiscreteKeys& dkeys, const DecisionTreeFactor& dt) { // SparseVector needs to know the maximum possible index, // so we compute the product of cardinalities. - size_t prod_cardinality = 1; + size_t cardinalityProduct = 1; for (auto&& [_, c] : dt.cardinalities()) { - prod_cardinality *= c; + cardinalityProduct *= c; } - Eigen::SparseVector sparse_table(prod_cardinality); + Eigen::SparseVector sparseTable(cardinalityProduct); size_t nrValues = 0; dt.visit([&nrValues](double x) { if (x > 0) nrValues += 1; }); - sparse_table.reserve(nrValues); + sparseTable.reserve(nrValues); std::set allKeys(dt.keys().begin(), dt.keys().end()); + /** + * @brief Functor which is called by the DecisionTree for each leaf. + * For each leaf value, we use the corresponding assignment to compute a + * corresponding index into a SparseVector. We then populate sparseTable with + * the value at the computed index. + * + * Takes advantage of the sparsity of the DecisionTree to be efficient. When + * merged branches are encountered, we enumerate over the missing keys. + * + */ auto op = [&](const Assignment& assignment, double p) { if (p > 0) { // Get all the keys involved in this assignment - std::set assignment_keys; + std::set assignmentKeys; for (auto&& [k, _] : assignment) { - assignment_keys.insert(k); + assignmentKeys.insert(k); } // Find the keys missing in the assignment std::vector diff; std::set_difference(allKeys.begin(), allKeys.end(), - assignment_keys.begin(), assignment_keys.end(), + assignmentKeys.begin(), assignmentKeys.end(), std::back_inserter(diff)); // Generate all assignments using the missing keys @@ -103,41 +118,43 @@ static Eigen::SparseVector ComputeLeafOrdering( for (auto&& key : diff) { extras.push_back({key, dt.cardinality(key)}); } - auto&& extra_assignments = DiscreteValues::CartesianProduct(extras); + auto&& extraAssignments = DiscreteValues::CartesianProduct(extras); - for (auto&& extra : extra_assignments) { + for (auto&& extra : extraAssignments) { // Create new assignment using the extra assignment - DiscreteValues updated_assignment(assignment); - updated_assignment.insert(extra); + DiscreteValues updatedAssignment(assignment); + updatedAssignment.insert(extra); // Generate index and add to the sparse vector. Eigen::Index idx = 0; - size_t prev_cardinality = 1; + size_t previousCardinality = 1; // We go in reverse since a DecisionTree has the highest label first - for (auto&& it = updated_assignment.rbegin(); - it != updated_assignment.rend(); it++) { - idx += prev_cardinality * it->second; - prev_cardinality *= dt.cardinality(it->first); + for (auto&& it = updatedAssignment.rbegin(); + it != updatedAssignment.rend(); it++) { + idx += previousCardinality * it->second; + previousCardinality *= dt.cardinality(it->first); } - sparse_table.coeffRef(idx) = p; + sparseTable.coeffRef(idx) = p; } } }; + // Visit each leaf in `dt` to get the Assignment and leaf value + // to populate the sparseTable. dt.visitWith(op); - return sparse_table; + return sparseTable; } /* ************************************************************************ */ TableFactor::TableFactor(const DiscreteKeys& dkeys, const DecisionTreeFactor& dtf) - : TableFactor(dkeys, ComputeLeafOrdering(dkeys, dtf)) {} + : TableFactor(dkeys, ComputeSparseTable(dkeys, dtf)) {} /* ************************************************************************ */ TableFactor::TableFactor(const DecisionTreeFactor& dtf) : TableFactor(dtf.discreteKeys(), - ComputeLeafOrdering(dtf.discreteKeys(), dtf)) {} + ComputeSparseTable(dtf.discreteKeys(), dtf)) {} /* ************************************************************************ */ TableFactor::TableFactor(const DiscreteConditional& c)