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

@@ -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,
@@ -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()) {
+    if (tree[nid].IsRoot()) {
-      // in distributed mode, the node's stats should be calculated from histogram, otherwise,
+      if (data_layout_ == kDenseDataZeroBased || data_layout_ == kDenseDataOneBased) {
-      // 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];*/
         const std::vector<uint32_t>& 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,6 +768,15 @@ 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
@@ -795,7 +788,6 @@ void QuantileHistMaker::Builder::InitNewNode(int nid,
             spliteval_->ComputeScore(parentid, snode_[nid].stats, snode_[nid].weight));
   }
 }
-}
 
 // enumerate the split values of specific feature
 void QuantileHistMaker::Builder::EnumerateSplit(int d_step,

src/tree/updater_quantile_hist.h

@@ -348,7 +348,6 @@ class QuantileHistMaker: public TreeUpdater {
     DataLayout data_layout_;
 
     TreeGrowingPerfMonitor perf_monitor;
-
     rabit::Reducer<GradStats, GradStats::Reduce> histred_;
   };
 

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(