Add basic unittests for gpu-hist method. #3785
Conversation
trivialfis
force-pushed
the
hist-tests
branch
from
b41523b to
4cf28c4
Compare
There was a problem hiding this comment.
Good job isolating some of the functionality for testing, this is not easy.
I am a little disturbed by how much manual initialisation of public member variables is necessary to set up these tests. I think it indicates design problems in our code. In my view these members should probably have been private with constructors controlling how they are allowed to be initialised or member functions for modification after initialisation.
It is a problem throughout xgboost that it is very easy to incorrectly initialize classes resulting in seg faults and other problems. If a class is correctly designed its interface should prevent misuse by other developers.
This is not a problem to be addressed by this PR, just some comments in general.
I can see a couple of files that only have minor formatting changes - please remove these from the PR, it makes it easier to manage.
tests/cpp/tree/test_gpu_hist.cu
Outdated
| {0.204452, 0.443453}, {0.878117, 0.229577}, | ||
| {0.0273876, 0.534414}, {0.670467, 0.913962}, | ||
| }; | ||
| thrust::device_vector<GradientPair> gpair (n_rows); |
There was a problem hiding this comment.
You can copy a std::vector to device_vector directly by assignment.
| TEST(GpuHist, ApplySplit) { | ||
| GPUHistMaker hist_maker = GPUHistMaker(); | ||
| int constexpr nid = 0; | ||
| int constexpr n_rows = 16; |
There was a problem hiding this comment.
My preferred syntax is "constexpr int", this is strange to read.
trivialfis
force-pushed
the
hist-tests
branch
from
4cf28c4 to
58d8eed
Compare
|
@trivialfis Nice job creating LCG random generator. Maybe in the future we can re-use it for other purposes. |
|
@hcho3 Thanks. maybe we can put it in dmlc-core. But it's not a thoroughly designed random generator, I imagine recovering the equation from output is not too hard to achieve. So I want to keep it for testing purpose only, in case others mis-use it in real algorithm. |
trivialfis
force-pushed
the
hist-tests
branch
from
58d8eed to
47ea459
Compare
* Split building histogram into separated class. * Extract `InitCompressedRow` definition. * Basic tests for gpu-hist. * Document the code more verbosely. * Removed `HistCutUnit`. * Removed some duplicated copies in `GPUHistMaker`. * Implement LCG and use it in tests.
trivialfis
force-pushed
the
hist-tests
branch
from
47ea459 to
cc80396
Compare
* Split building histogram into separated class. * Extract `InitCompressedRow` definition. * Basic tests for gpu-hist. * Document the code more verbosely. * Removed `HistCutUnit`. * Removed some duplicated copies in `GPUHistMaker`. * Implement LCG and use it in tests.
* Split building histogram into separated class. * Extract `InitCompressedRow` definition. * Basic tests for gpu-hist. * Document the code more verbosely. * Removed `HistCutUnit`. * Removed some duplicated copies in `GPUHistMaker`. * Implement LCG and use it in tests.
InitCompressedRowdefinition.This PR is to add some basic unittests for gpu-hist method, for later refactor (using span, reducing memory requirement, etc.) needs.
Trying to create the unit tests somehow shows that each step in the algorithm depends on different and large amount of data pieces. I hope we can come up with a better solution to handle these data (and parameters) dependencies before adding more extensive tests.