Separate Depthwidth and Lossguide growing policy in fast histogram #4102

CodingCat · 2019-02-05T13:33:29Z

it's a following PR of #4011 which separates depthwidth and loss guide growing policy in fast histogram per our discussion in #4077

closes #4077

@trivialfis @hcho3 @RAMitchell please help to review

jvm-packages/dev/build.sh

hcho3 · 2019-02-07T06:48:56Z

@CodingCat Can you rebase against the latest master?

CodingCat · 2019-02-08T05:03:49Z

@trivialfis @hcho3 @RAMitchell ping for review

CodingCat · 2019-02-10T21:27:48Z

@trivialfis @hcho3 @RAMitchell ping for review

hcho3 · 2019-02-10T21:32:27Z

Will review today

trivialfis · 2019-02-10T22:26:08Z

Glanced the changes. Will leave comments today. Got into some troubles with my network access recently. Sorry for the delay.

RAMitchell

I think we need a better, more consistent logic around whether to build histograms or use the subtraction trick. For example could we push a lot of this logic into some histogram class that maintains the state of if it needs to build a new histogram or can use the subtraction trick, or needs to sync? Then we just ask it for the histogram for a particular node and it decides what to do? This is just an idea I would like some more thoughts @CodingCat @hcho3.

src/common/column_matrix.h

src/tree/updater_quantile_hist.cc

src/tree/updater_quantile_hist.h

hcho3

I did a first pass over this PR. I agree with the overall decision to split loss-guide and depth-wise strategies.

The complex accounting for sibling relations (left_to_right_siblings and right_to_left_siblings) can potentially be simplified. Doesn't each tree node already encode whether it's a left child or right child? See

xgboost/include/xgboost/tree_model.h

Line 132 in 99a2904

bool IsLeftChild() const {

src/common/column_matrix.h

src/tree/updater_quantile_hist.h

src/tree/updater_quantile_hist.cc

CodingCat · 2019-02-11T08:42:53Z

@RAMitchell @hcho3 thank you very much for the review, I have addressed most of the comments (will continue working on the performance monitor class tmr )

trivialfis

I concur with @RAMitchell that we can put more thoughts in general structure of histogram building code. Since this PR is about separating two strategists, I tried to compare two ExpandWith* side by side, and I think there's a slight chance that we can modify share the code of split evaluating and new node initializing between two methods. I'm ok with merging this after issues mentioned in comments got resolved. But it might be better that we make more clarity to the code before merging.

...kages/xgboost4j-spark/src/test/scala/ml/dmlc/xgboost4j/scala/spark/XGBoostGeneralSuite.scala

src/common/hist_util.cc

src/tree/updater_quantile_hist.cc

src/tree/updater_quantile_hist.h

src/tree/updater_quantile_hist.cc

trivialfis · 2019-02-11T18:09:04Z

Seems the comments got a little bit messy. It's because my machine got frozen during a review. After rebooting, the previous review was not visible to me so I have to do it again. After publishing it the previous somehow shows up ... If you find similar comments, please ignore one of them.

CodingCat · 2019-02-12T01:06:39Z

I think I have addressed all comments, ready for the next round of review

RAMitchell · 2019-02-12T01:52:08Z

Is there some reason you didn't use the existing Monitor class?

xgboost/src/common/timer.h

Line 41 in 99a2904

* \struct Monitor

CodingCat · 2019-02-12T05:09:28Z

@RAMitchell I was not aware of this class...but when I take a look at Monitor class there, I found it has several differences with the performance monitoring here, e.g. the format of the report and the log level, I am a bit hesitated about whether we should change the behavior of performance reporting either in hist or in other places using that class

I personally vote to unify the performance monitoring across updaters after this release

src/tree/updater_quantile_hist.cc

hcho3

LGTM. I like the overall organization of the code. I have some minor stylistic comments

src/tree/updater_quantile_hist.h

hcho3 · 2019-02-12T21:20:54Z

src/tree/updater_quantile_hist.cc

+      << "tree_method=hist does not support multiple roots at this moment";
+  if (param_.grow_policy == TrainParam::kLossGuide) {
+     ExpandWithLossGuide(gmat, gmatb, column_matrix, p_fmat, p_tree, gpair_h);
+    while (!qexpand_loss_guided_->empty()) {


Let's just remove this loop, along with the 3-line comments?

hcho3 · 2019-02-12T21:21:07Z

src/tree/updater_quantile_hist.cc

+    }
+  } else {
+    ExpandWithDepthWidth(gmat, gmatb, column_matrix, p_fmat, p_tree, gpair_h);
+  }

  // set all the rest expanding nodes to leaf


NVM, let's just remove the comment

src/tree/updater_quantile_hist.cc

src/tree/updater_quantile_hist.h

src/tree/updater_quantile_hist.cc

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

hcho3

LGTM

hcho3 · 2019-02-13T19:49:33Z

src/tree/updater_quantile_hist.cc

+
+  unsigned timestamp = 0;
+  int num_leaves = 0;
+
  for (int nid = 0; nid < p_tree->param.num_roots; ++nid) {


Once this is merged, I will file a follow-up PR to deprecate num_roots parameter.

CodingCat · 2019-02-13T20:56:13Z

thanks for the review @trivialfis @hcho3 @RAMitchell

Denisevi4 · 2019-02-14T04:44:19Z

So depthwidth is now going to be per-level? :(

That's unfortunate, because we use depthwidth and the per-nodeness of fast histogram to do feature selection - addding penalty to loss change for unused features. Once feature is used, no penalty is applied. With per-levelness it wouldn't work well especially for deep trees. Was it really worth it?

CodingCat · 2019-02-14T05:27:08Z

@Denisevi4 it helps in reducing communication overhead in distributed training and serves as the base for improving multi-cores performance in the next release by increasing maximum parallelism

hcho3 · 2019-02-14T05:38:43Z

@Denisevi4 The per-nodeness of fast histogram was not so good for performance, due to 1) extra syncing needed in distributed setting, and 2) lack of parallelism in multi-core CPU

hcho3 reviewed Feb 5, 2019

View reviewed changes

jvm-packages/dev/build.sh Show resolved Hide resolved

CodingCat force-pushed the dist_fast_histogram_per_level branch from 38c8da2 to be17c1f Compare February 7, 2019 17:27

RAMitchell reviewed Feb 10, 2019

View reviewed changes

hcho3 reviewed Feb 11, 2019

View reviewed changes

trivialfis reviewed Feb 11, 2019

View reviewed changes

CodingCat and others added 18 commits February 11, 2019 13:14

fix scalastyle error

0c71d3c

add back train method but mark as deprecated

2397ab5

fix scalastyle error

fd3b1ef

add back train method but mark as deprecated

fac8d16

fix scalastyle error

558ac6d

add back train method but mark as deprecated

db72c4d

fix scalastyle error

11d5718

init

ad957fe

more changes

e0c6f4a

temp

09fb228

update

cf4add3

udpate rabit

6924891

change the histogram

6adabca

update kfactor

22df6da

sync per node stats

2f9f664

temp

b99080a

update

764dddc

final

3bbd77d

CodingCat force-pushed the dist_fast_histogram_per_level branch from 5fc0b11 to 79f7d31 Compare February 11, 2019 21:15

Nan Zhu added 5 commits February 11, 2019 14:25

fix failed tests

76e048e

wrap perf timers with class

1749682

fix lint

df67e2a

fix num_leaves count

c128cf1

fix indention

0c9e034

hcho3 reviewed Feb 12, 2019

View reviewed changes

src/tree/updater_quantile_hist.cc Outdated Show resolved Hide resolved

hcho3 reviewed Feb 12, 2019

View reviewed changes

hcho3 and others added 8 commits February 13, 2019 00:02

Update src/tree/updater_quantile_hist.cc

e4211d5

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

Update src/tree/updater_quantile_hist.h

b38323b

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

Update src/tree/updater_quantile_hist.cc

6b0dc06

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

Update src/tree/updater_quantile_hist.cc

0535144

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

Update src/tree/updater_quantile_hist.cc

a3c9a68

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

Update src/tree/updater_quantile_hist.h

773e8a3

Co-Authored-By: CodingCat <CodingCat@users.noreply.github.com>

merge

6e07047

fix compilation

fffc585

CodingCat mentioned this pull request Feb 13, 2019

distributed hist tree method not working? #4127

Closed

hcho3 approved these changes Feb 13, 2019

View reviewed changes

trivialfis approved these changes Feb 13, 2019

View reviewed changes

CodingCat merged commit c18a366 into dmlc:master Feb 13, 2019

hcho3 mentioned this pull request Mar 4, 2019

[RFC] Version 0.82 release candidate #4201

Merged

trivialfis mentioned this pull request Mar 18, 2019

Use Monitor in quantile hist. #4273

Merged

lock bot locked as resolved and limited conversation to collaborators May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Depthwidth and Lossguide growing policy in fast histogram #4102

Separate Depthwidth and Lossguide growing policy in fast histogram #4102

CodingCat commented Feb 5, 2019 •

edited

Loading

hcho3 commented Feb 7, 2019

CodingCat commented Feb 8, 2019

CodingCat commented Feb 10, 2019

hcho3 commented Feb 10, 2019

trivialfis commented Feb 10, 2019

RAMitchell left a comment

hcho3 left a comment

CodingCat commented Feb 11, 2019

trivialfis left a comment

trivialfis commented Feb 11, 2019 •

edited

Loading

CodingCat commented Feb 12, 2019

RAMitchell commented Feb 12, 2019

CodingCat commented Feb 12, 2019

hcho3 left a comment

hcho3 Feb 12, 2019

hcho3 Feb 12, 2019

hcho3 left a comment

hcho3 Feb 13, 2019

CodingCat commented Feb 13, 2019

Denisevi4 commented Feb 14, 2019

CodingCat commented Feb 14, 2019 •

edited

Loading

hcho3 commented Feb 14, 2019

Separate Depthwidth and Lossguide growing policy in fast histogram #4102

Separate Depthwidth and Lossguide growing policy in fast histogram #4102

Conversation

CodingCat commented Feb 5, 2019 • edited Loading

hcho3 commented Feb 7, 2019

CodingCat commented Feb 8, 2019

CodingCat commented Feb 10, 2019

hcho3 commented Feb 10, 2019

trivialfis commented Feb 10, 2019

RAMitchell left a comment

Choose a reason for hiding this comment

hcho3 left a comment

Choose a reason for hiding this comment

CodingCat commented Feb 11, 2019

trivialfis left a comment

Choose a reason for hiding this comment

trivialfis commented Feb 11, 2019 • edited Loading

CodingCat commented Feb 12, 2019

RAMitchell commented Feb 12, 2019

CodingCat commented Feb 12, 2019

hcho3 left a comment

Choose a reason for hiding this comment

hcho3 Feb 12, 2019

Choose a reason for hiding this comment

hcho3 Feb 12, 2019

Choose a reason for hiding this comment

hcho3 left a comment

Choose a reason for hiding this comment

hcho3 Feb 13, 2019

Choose a reason for hiding this comment

CodingCat commented Feb 13, 2019

Denisevi4 commented Feb 14, 2019

CodingCat commented Feb 14, 2019 • edited Loading

hcho3 commented Feb 14, 2019

CodingCat commented Feb 5, 2019 •

edited

Loading

trivialfis commented Feb 11, 2019 •

edited

Loading

CodingCat commented Feb 14, 2019 •

edited

Loading