-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distributed Fast Histogram Algorithm #4011
Conversation
...ages/xgboost4j-spark/src/main/scala/ml/dmlc/xgboost4j/scala/spark/params/BoosterParams.scala
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #4011 +/- ##
============================================
+ Coverage 57.24% 60.73% +3.49%
============================================
Files 190 130 -60
Lines 15045 11718 -3327
Branches 527 0 -527
============================================
- Hits 8612 7117 -1495
+ Misses 6176 4601 -1575
+ Partials 257 0 -257
Continue to review full report at Codecov.
|
ping? @RAMitchell @hcho3 @yanboliang |
Native code looks good! Should there be a test here or are the Java tests enough? |
@CodingCat I'm now back from winter vacation. I'll review once #3957 is merged. |
I am testing with our internal dataset while we get 1.5-2X speedup, I found that the training accuracy of fast-histogram is a bit lower than approx, anyone see the same thing before? |
and when we set col_sample_bytree, fast histogram is even slower than approx, I am investigating if everything is fine in that part |
aee9bfc
to
5fc66db
Compare
Hi @CodingCat I would be very happy to do some tests on our datasets - is it possible to grab the packages for that PR from somewhere? Or should I recompile everything on my own? |
Thanks, @troszok You can fetch the version with the distributed fast histo support with the approach in https://xgboost.readthedocs.io/en/latest/jvm/xgboost4j_spark_tutorial.html#refer-to-xgboost4j-spark-dependency (search the version number is 0.82-SNAPSHOT |
5fc66db
to
853a758
Compare
jvm-packages/xgboost4j-spark/src/main/scala/ml/dmlc/xgboost4j/scala/spark/XGBoost.scala
Show resolved
Hide resolved
@troszok any update about your test for accuracy? |
|
@hcho3 ping for review? |
853a758
to
f326157
Compare
src/common/random.h
Outdated
std::sort(new_features.begin(), new_features.end()); | ||
|
||
new_features.resize(static_cast<unsigned long>(n)); | ||
// std::sort(new_features.begin(), new_features.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit confused by the old code here, was there any reason to sort just after we had shuffled the features?
Then could you explain why we need the ser/deser compared to what was happening before?
@Liuhaoge this is a discussion about a new upcoming feature in the codebase, asking questions here will not get you anywhere and is diverting from the main topic. Once the feature has been merged I'd recommend asking questions about usage on the discussion board, we try to use Github for bug reporting and development discussions. |
@thvasilo Actually it might be reasonable to include some documentations about added features in this PR. :) Our documentation is not very thorough. |
@trivialfis Good point, we had that as a requirement for merging PR's that introduce new features in other code-bases I've worked on. |
@CodingCat Did you establish an interface to use Monotonic Constrains in Scala? How to use that in distributed environment? |
Hello @CodingCat, I tried running an experiment creating a local cluster with 6 workers today. Using the My configuration file (higgs.conf) looks like this:
And I use this to run the command: Looking at the output seems like
Edit: I guess this is related to #4078 , maybe I missed something there. |
d42ac2c
to
d3b312a
Compare
rebased for running with fixed travis |
@trivialfis doc is updated |
@RAMitchell @trivialfis I'm seeing a memory error in the GPU test. Any idea why?
|
@hcho3 you have time to review? There is a following PR based on this to separate depthwidth and lossguide |
@CodingCat Okay, I'll take a quick look today |
@hcho3 any update? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodingCat LGTM, as far as I'm aware of. I really appreciate your work for updating the fast hist algorithm.
@@ -105,6 +105,7 @@ class QuantileHistMaker: public TreeUpdater { | |||
} else { | |||
hist_builder_.BuildHist(gpair, row_indices, gmat, hist); | |||
} | |||
this->histred_.Allreduce(hist.begin, hist_builder_.GetNumBins()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pleasantly surprised that distributed implementation is this succinct and concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the all reduce interface is easy to use and we only need to make some additional care to substract tricks in distributed mode
basically need to remove several implementation assumption in fast histogram algorithm
@hcho3 please help to review
I will test with our internal dataset