DecisionTree Refactor #1155
DecisionTree Refactor #1155
Conversation
|
@dellaert hope this PR is up to your expectations. |
|
@dellaert gentle reminder to take a look at this when you can :) |
| /** | ||
| * Functor performing depth-first visit without Assignment<L> argument. | ||
| * | ||
| * NOTE: We differentiate between leaves and assignments. Concretely, a 3 |
There was a problem hiding this comment.
So, what does this Functor do? Leaves, right? And should it not now pass the number of assignments captured by the leaf?
There was a problem hiding this comment.
The VisitLeaf functor passes the Leaf object which has the number of assignments recorded in it.
gtsam/discrete/DecisionTree-inl.h
Outdated
| @@ -678,7 +680,14 @@ namespace gtsam { | |||
| } | |||
|
|
|||
| /****************************************************************************/ | |||
| // Functor performing depth-first visit without Assignment<L> argument. | |||
| /** | |||
| * Functor performing depth-first visit without Assignment<L> argument. | |||
gtsam/discrete/DecisionTree-inl.h
Outdated
| @@ -763,12 +775,14 @@ namespace gtsam { | |||
| } | |||
|
|
|||
| /****************************************************************************/ | |||
| // labels is just done with a visit | |||
| // Get (partial) labels by performing a visit. | |||
There was a problem hiding this comment.
Explain more. Maybe with example.
| * | ||
| * @note Due to pruning, leaves might not exhaust choices. | ||
| * | ||
| * |
There was a problem hiding this comment.
I think these functions passed in should now also take an argument for the number of assignments captured in a particular leaf.
There was a problem hiding this comment.
I added visitLeaf as an alternative to that. Should I remove that and update the rest to do this (will be API breaking)?
There was a problem hiding this comment.
Shoot, I merged too soon! I just noticed this new visitLeaf, which in this PR seems identical??
There was a problem hiding this comment.
And the examples are wrong I think... We can talk about it in our meeting.
There was a problem hiding this comment.
Okay! Though I don't want to spend more than 5 minutes on this since it is secondary to the workshop paper.
Refactored
DecisionTreeto:DT_NO_PRUNINGtoGTSAM_DT_NO_PRUNINGin the event we decide to expose it via CMake.