From 1dac5e24108890c946c5a4673f0d1fa4d6125943 Mon Sep 17 00:00:00 2001 From: Nan Zhu Date: Mon, 18 Feb 2019 13:45:30 -0800 Subject: [PATCH] more correct way to build node stats in distributed fast hist (#4140) * add back train method but mark as deprecated * add back train method but mark as deprecated * add back train method but mark as deprecated * fix scalastyle error * fix scalastyle error * fix scalastyle error * fix scalastyle error * more changes * temp * update * udpate rabit * change the histogram * update kfactor * sync per node stats * temp * update * final * code clean * update rabit * more cleanup * fix errors * fix failed tests * enforce c++11 * broadcast subsampled feature correctly * init col * temp * col sampling * fix histmastrix init * fix col sampling * remove cout * fix out of bound access * fix core dump remove core dump file * update * add fid * update * revert some changes * temp * temp * pass all tests * bring back some tests * recover some changes * fix lint issue * enable monotone and interaction constraints * don't specify default for monotone and interactions * recover column init part * more recovery * fix core dumps * code clean * revert some changes * fix test compilation issue * fix lint issue * resolve compilation issue * fix issues of lint caused by rebase * fix stylistic changes and change variable names * modularize depth width * address the comments * fix failed tests * wrap perf timers with class * temp * pass all lossguide * pass tests * add comments * more changes * use separate flow for single and tests * add test for lossguide hist * remove duplications * syncing stats for only once * recover more changes * recover more changes * fix root-stats * simplify code * remove outdated comments --- src/tree/updater_quantile_hist.cc | 54 ++++++++++------------- src/tree/updater_quantile_hist.h | 1 - tests/python/test_monotone_constraints.py | 20 +++++++-- 3 files changed, 40 insertions(+), 35 deletions(-) diff --git a/src/tree/updater_quantile_hist.cc b/src/tree/updater_quantile_hist.cc index f479e85cc..b9395b517 100644 --- a/src/tree/updater_quantile_hist.cc +++ b/src/tree/updater_quantile_hist.cc @@ -117,8 +117,8 @@ void QuantileHistMaker::Builder::BuildLocalHistograms( RegTree::Node &node = (*p_tree)[nid]; if (rabit::IsDistributed()) { if (node.IsRoot() || node.IsLeftChild()) { - // in distributed setting, we always calcuate from left child or root node hist_.AddHistRow(nid); + // in distributed setting, we always calculate from left child or root node BuildHist(gpair_h, row_set_collection_[nid], gmat, gmatb, hist_[nid], false); if (!node.IsRoot()) { nodes_for_subtraction_trick_[(*p_tree)[node.Parent()].RightChild()] = nid; @@ -144,7 +144,6 @@ void QuantileHistMaker::Builder::BuildLocalHistograms( (*sync_count)++; (*starting_index) = std::min((*starting_index), nid); } else if (node.IsRoot()) { - // root node hist_.AddHistRow(nid); BuildHist(gpair_h, row_set_collection_[nid], gmat, gmatb, hist_[nid], false); (*sync_count)++; @@ -211,8 +210,6 @@ void QuantileHistMaker::Builder::EvaluateSplits( } } - - void QuantileHistMaker::Builder::ExpandWithDepthWidth( const GHistIndexMatrix &gmat, const GHistIndexBlockMatrix &gmatb, @@ -234,7 +231,7 @@ void QuantileHistMaker::Builder::ExpandWithDepthWidth( SyncHistograms(starting_index, sync_count, p_tree); BuildNodeStats(gmat, p_fmat, p_tree, gpair_h); EvaluateSplits(gmat, column_matrix, p_fmat, p_tree, &num_leaves, depth, ×tamp, - &temp_qexpand_depth); + &temp_qexpand_depth); // clean up qexpand_depth_wise_.clear(); nodes_for_subtraction_trick_.clear(); @@ -290,11 +287,12 @@ void QuantileHistMaker::Builder::ExpandWithLossGuide( this->ApplySplit(nid, gmat, column_matrix, hist_, *p_fmat, p_tree); perf_monitor.UpdatePerfTimer(TreeGrowingPerfMonitor::timer_name::APPLY_SPLIT); - perf_monitor.TickStart(); const int cleft = (*p_tree)[nid].LeftChild(); const int cright = (*p_tree)[nid].RightChild(); hist_.AddHistRow(cleft); hist_.AddHistRow(cright); + + perf_monitor.TickStart(); if (rabit::IsDistributed()) { // in distributed mode, we need to keep consistent across workers BuildHist(gpair_h, row_set_collection_[cleft], gmat, gmatb, hist_[cleft], true); @@ -316,7 +314,7 @@ void QuantileHistMaker::Builder::ExpandWithLossGuide( bst_uint featureid = snode_[nid].best.SplitIndex(); spliteval_->AddSplit(nid, cleft, cright, featureid, snode_[cleft].weight, snode_[cright].weight); - perf_monitor.UpdatePerfTimer(TreeGrowingPerfMonitor::timer_name::APPLY_SPLIT); + perf_monitor.UpdatePerfTimer(TreeGrowingPerfMonitor::timer_name::INIT_NEW_NODE); perf_monitor.TickStart(); this->EvaluateSplit(cleft, gmat, hist_, *p_fmat, *p_tree); @@ -754,23 +752,9 @@ void QuantileHistMaker::Builder::InitNewNode(int nid, { auto& stats = snode_[nid].stats; GHistRow hist = hist_[nid]; - if (rabit::IsDistributed()) { - // in distributed mode, the node's stats should be calculated from histogram, otherwise, - // we will have wrong results in EnumerateSplit() - // here we take the last feature in cut - auto begin = hist.data(); - for (size_t i = gmat.cut.row_ptr[0]; i < gmat.cut.row_ptr[1]; i++) { - stats.Add(begin[i].sum_grad, begin[i].sum_hess); - } - } else { - if (data_layout_ == kDenseDataZeroBased || data_layout_ == kDenseDataOneBased || - rabit::IsDistributed()) { - /* specialized code for dense data - For dense data (with no missing value), - the sum of gradient histogram is equal to snode[nid] - GHistRow hist = hist_[nid];*/ + if (tree[nid].IsRoot()) { + if (data_layout_ == kDenseDataZeroBased || data_layout_ == kDenseDataOneBased) { const std::vector& row_ptr = gmat.cut.row_ptr; - const uint32_t ibegin = row_ptr[fid_least_bins_]; const uint32_t iend = row_ptr[fid_least_bins_ + 1]; auto begin = hist.data(); @@ -784,16 +768,24 @@ void QuantileHistMaker::Builder::InitNewNode(int nid, stats.Add(gpair[*it]); } } + histred_.Allreduce(&snode_[nid].stats, 1); + } else { + int parent_id = tree[nid].Parent(); + if (tree[nid].IsLeftChild()) { + snode_[nid].stats = snode_[parent_id].best.left_sum; + } else { + snode_[nid].stats = snode_[parent_id].best.right_sum; + } } + } - // calculating the weights - { - bst_uint parentid = tree[nid].Parent(); - snode_[nid].weight = static_cast( - spliteval_->ComputeWeight(parentid, snode_[nid].stats)); - snode_[nid].root_gain = static_cast( - spliteval_->ComputeScore(parentid, snode_[nid].stats, snode_[nid].weight)); - } + // calculating the weights + { + bst_uint parentid = tree[nid].Parent(); + snode_[nid].weight = static_cast( + spliteval_->ComputeWeight(parentid, snode_[nid].stats)); + snode_[nid].root_gain = static_cast( + spliteval_->ComputeScore(parentid, snode_[nid].stats, snode_[nid].weight)); } } diff --git a/src/tree/updater_quantile_hist.h b/src/tree/updater_quantile_hist.h index 288fbc867..c78dcad90 100644 --- a/src/tree/updater_quantile_hist.h +++ b/src/tree/updater_quantile_hist.h @@ -348,7 +348,6 @@ class QuantileHistMaker: public TreeUpdater { DataLayout data_layout_; TreeGrowingPerfMonitor perf_monitor; - rabit::Reducer histred_; }; diff --git a/tests/python/test_monotone_constraints.py b/tests/python/test_monotone_constraints.py index bb099dd17..aa2b0c9a4 100644 --- a/tests/python/test_monotone_constraints.py +++ b/tests/python/test_monotone_constraints.py @@ -63,7 +63,7 @@ class TestMonotoneConstraints(unittest.TestCase): # first check monotonicity for the 'exact' tree method params_for_constrained_exact_method = { - 'tree_method': 'exact', 'silent': 1, + 'tree_method': 'exact', 'verbosity': 1, 'monotone_constraints': '(1, -1)' } constrained_exact_method = xgb.train( @@ -71,11 +71,25 @@ class TestMonotoneConstraints(unittest.TestCase): ) assert is_correctly_constrained(constrained_exact_method) - def test_monotone_constraints_for_hist_tree_method(self): + def test_monotone_constraints_for_depthwise_hist_tree_method(self): # next check monotonicity for the 'hist' tree method params_for_constrained_hist_method = { - 'tree_method': 'hist', 'silent': 1, + 'tree_method': 'hist', 'verbosity': 1, + 'monotone_constraints': '(1, -1)' + } + constrained_hist_method = xgb.train( + params_for_constrained_hist_method, training_dset + ) + + assert is_correctly_constrained(constrained_hist_method) + + def test_monotone_constraints_for_lossguide_hist_tree_method(self): + + # next check monotonicity for the 'hist' tree method + params_for_constrained_hist_method = { + 'tree_method': 'hist', 'verbosity': 1, + 'grow_policy': 'lossguide', 'monotone_constraints': '(1, -1)' } constrained_hist_method = xgb.train(