Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decision Tree Improvements #1542

Merged
merged 16 commits into from
Jun 28, 2023
Merged

Decision Tree Improvements #1542

merged 16 commits into from
Jun 28, 2023

Conversation

varunagrawal
Copy link
Collaborator

  1. Unique is now bottom-up, since I found another test case where it was incorrect and this fixes it.
  2. Print the number of assignments inside square brackets when printing a DecisionTree.
  3. Change GTSAM_DT_NO_PRUNING to GTSAM_DT_MERGING so it disambiguates from Hybrid pruning.
  4. Expose GTSAM_DT_MERGING via CMake.

@varunagrawal varunagrawal self-assigned this Jun 10, 2023
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but see my question. email me when I should look again

@@ -549,15 +544,15 @@ namespace gtsam {
template<typename L, typename Y>
template<typename Iterator> DecisionTree<L, Y>::DecisionTree(
Iterator begin, Iterator end, const L& label) {
root_ = compose(begin, end, label);
root_ = Choice::Unique(compose(begin, end, label));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do all these have a Unique now? That was not detailed in the PR comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I undid that. Looks like it was a remnant of debugging.

@@ -199,47 +200,41 @@ namespace gtsam {
}

/// If all branches of a choice node f are the same, just return a branch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO(Varun): Elaborate more on how this works.

@varunagrawal
Copy link
Collaborator Author

@dellaert so I am really struggling with implementing this function correctly in the decision tree

// Applying binary operator to two leaves results in a leaf
  NodePtr apply_g_op_fL(const Leaf& fL, const Binary& op) const override {
    // fL op gL
    // TODO(Varun) Is the nrAssignments setting correct?
    NodePtr h(new Leaf(op(fL.constant_, constant_), fL.nrAssignments()));
    return h;
  }

I only need nrAssignments for pruning in the DecisionTreeFactor, so I replaced that with a loop over all the assignments in the cartesian product (which is a bit slower)

auto allValues = DiscreteValues::CartesianProduct(this->discreteKeys());
for (auto&& val : allValues) {
  double prob = (*this)(val);
  probabilities.push_back(prob);
}

It seems that there is no time improvement, since the test case is now taking 131.93 seconds compared to 117 seconds with the cartesian product taking up 12.8236 seconds (which accounts for the time difference).

My conclusion is that perhaps the DecisionTreeFactor is inherently non-scalable, which means I should perhaps move to the TableFactor.

@varunagrawal
Copy link
Collaborator Author

I am contemplating removing the nrAssignments variable since I introduced it just for pruning and it is not being correctly computed. :(

@varunagrawal varunagrawal merged commit b86696a into fix-1496 Jun 28, 2023
@varunagrawal varunagrawal deleted the decisiontree-improvements branch June 28, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants