From 5d74578095e1414cfcb62f9732165842f25b81ca Mon Sep 17 00:00:00 2001 From: Philip Cho Date: Sat, 21 Jan 2017 12:02:29 -0800 Subject: [PATCH] Disallow multiple roots for tree_method=hist (#1979) As discussed in issue #1978, tree_method=hist ignores the parameter param.num_roots; it simply assumes that the tree has only one root. In particular, when InitData() method initializes row_set_collection_, it simply assigns all rows to node 0, the value that's hard-coded. For now, the updater will simply fail when num_roots exceeds 1. I will revise the updater soon to support multiple roots. --- src/tree/tree_updater.cc | 1 + src/tree/updater_fast_hist.cc | 3 +++ 2 files changed, 4 insertions(+) diff --git a/src/tree/tree_updater.cc b/src/tree/tree_updater.cc index ca04a2c84..faa850f5b 100644 --- a/src/tree/tree_updater.cc +++ b/src/tree/tree_updater.cc @@ -29,6 +29,7 @@ DMLC_REGISTRY_LINK_TAG(updater_colmaker); DMLC_REGISTRY_LINK_TAG(updater_skmaker); DMLC_REGISTRY_LINK_TAG(updater_refresh); DMLC_REGISTRY_LINK_TAG(updater_prune); +DMLC_REGISTRY_LINK_TAG(updater_fast_hist); DMLC_REGISTRY_LINK_TAG(updater_histmaker); DMLC_REGISTRY_LINK_TAG(updater_sync); } // namespace tree diff --git a/src/tree/updater_fast_hist.cc b/src/tree/updater_fast_hist.cc index 13a651d9b..9d5afd1d9 100644 --- a/src/tree/updater_fast_hist.cc +++ b/src/tree/updater_fast_hist.cc @@ -139,6 +139,9 @@ class FastHistMaker: public TreeUpdater { tstart = dmlc::GetTime(); this->InitData(gmat, gpair, *p_fmat, *p_tree); time_init_data = dmlc::GetTime() - tstart; + // FIXME(hcho3): this code is broken when param.num_roots > 1. Please fix it + CHECK_EQ(p_tree->param.num_roots, 1) + << "tree_method=hist does not support multiple roots at this moment"; for (int nid = 0; nid < p_tree->param.num_roots; ++nid) { tstart = dmlc::GetTime(); hist_.AddHistRow(nid);