Github user orhankislal commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/142#discussion_r123577006
--- Diff: src/modules/recursive_partitioning/DT_impl.hpp ---
@@ -486,8 +485,18 @@ DecisionTree<Container>::expand(const Accumulator
&state,
Index stats_i = static_cast<Index>(state.stats_lookup(i));
assert(stats_i >= 0);
- // 1. Set the prediction for current node from stats of all
rows
- predictions.row(current) = state.node_stats.row(stats_i);
+ if (statCount(predictions.row(current)) !=
+ statCount(state.node_stats.row(stats_i))){
+ // Predictions for each node is set by its parent using
stats
+ // recorded while training parent node. These stats do not
include
+ // rows that had a NULL value for the primary split
feature.
+ // The NULL count is included in the 'node_stats' while
training
+ // current node. Further, presence of NULL rows indicate
that
+ // stats used for deciding 'children_wont_split' are
inaccurate.
+ // Hence avoid using the flag to decide termination.
+ predictions.row(current) = state.node_stats.row(stats_i);
+ children_wont_split = false;
+ }
--- End diff --
Should we move the sections 2 and 3 under the else clause of this if check
(except the `children_not_allocated` check at line 559)? Since
`children_wont_split` is set to false, the calculation at line 564 should be
redundant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
