-
Notifications
You must be signed in to change notification settings - Fork 544
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
[REVIEW] Symbolic Regression/Classification C/C++ #3638
[REVIEW] Symbolic Regression/Classification C/C++ #3638
Conversation
Can one of the admins verify this patch? |
…b.com/vimarsh6739/cuml into fea-ext-genetic-programming-internals(reverse pull)
aba3cda
to
b756c13
Compare
rerun tests |
2 similar comments
rerun tests |
rerun tests |
@dantegd any ideas why the CI is still failing here? |
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.
Hi @venkywonka, thanks for fixing the tests. I have one more suggestion: not to wrap device_uvectors
into unique_ptr
.
std::vector<float> h_trainwts; | ||
std::vector<float> h_testwts; | ||
|
||
std::unique_ptr<rmm::device_uvector<float>> d_train, d_trainlab, d_test, d_testlab, d_trainwts, |
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.
Using unique_ptr
is redundant. rmm::device_uvector
already manages the lifetime of the underlying data. If the reason for wrapping it into unique_ptr
, is that you do not know the size / do not have a stream at construction time, then I would simple suggest to construct it with 0 size on stream 0, and resize it later.
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.
Wrapping it around wasn't my first choice either tamas, but since rmm::device_uvector
does not have a default constructor, but the gtest struct required one, the build was failing with a "default constructor cannot be referenced -- it is a deleted function"
error. Initializing it as rmm::device_uvector<float> d_train(0,0);
and resize()
-ing it later didn't help either. So I found a way as per here that wrapped it with unique_ptr
that seemed to fix the build errors. Is the rmm::device_uvector<float> d_train(0, 0);
what you meant?
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.
If you just read the comment below what you have cited, you will see that even in that case it was possible to get rid of the extra unique_ptr wrappers. This is how it looks: link. And this is how it would look for the current PR:
diff --git a/cpp/test/sg/genetic/evolution_test.cu b/cpp/test/sg/genetic/evolution_test.cu
index d131c185a..b48b0f4e2 100644
--- a/cpp/test/sg/genetic/evolution_test.cu
+++ b/cpp/test/sg/genetic/evolution_test.cu
@@ -39,7 +39,7 @@ namespace genetic {
*/
class GeneticEvolutionTest : public ::testing::Test {
public:
- void SetUp() override
+ GeneticEvolutionTest() : d_train(0, stream)
{
ML::Logger::get().setLevel(CUML_LEVEL_INFO);
CUDA_CHECK(cudaStreamCreate(&stream));
@@ -62,7 +62,7 @@ class GeneticEvolutionTest : public ::testing::Test {
h_testwts.resize(n_tst_rows, 1.0f);
// Initialize device memory
- d_train = std::make_unique<rmm::device_uvector<float>>(n_cols * n_tr_rows, stream);
+ d_train.resize(n_cols * n_tr_rows, stream);
d_trainlab = std::make_unique<rmm::device_uvector<float>>(n_tr_rows, stream);
d_test = std::make_unique<rmm::device_uvector<float>>(n_cols * n_tst_rows, stream);
d_testlab = std::make_unique<rmm::device_uvector<float>>(n_tst_rows, stream);
@@ -70,7 +70,7 @@ class GeneticEvolutionTest : public ::testing::Test {
d_testwts = std::make_unique<rmm::device_uvector<float>>(n_tst_rows, stream);
// Memcpy HtoD
- CUDA_CHECK(cudaMemcpyAsync(d_train->data(),
+ CUDA_CHECK(cudaMemcpyAsync(d_train.data(),
h_train.data(),
n_cols * n_tr_rows * sizeof(float),
cudaMemcpyHostToDevice,
@@ -105,7 +105,7 @@ class GeneticEvolutionTest : public ::testing::Test {
void TearDown() override { CUDA_CHECK(cudaStreamDestroy(stream)); }
raft::handle_t handle;
- cudaStream_t stream;
+ cudaStream_t stream = 0;
param hyper_params;
// Some mini-dataset constants
@@ -244,8 +244,8 @@ class GeneticEvolutionTest : public ::testing::Test {
std::vector<float> h_trainwts;
std::vector<float> h_testwts;
- std::unique_ptr<rmm::device_uvector<float>> d_train, d_trainlab, d_test, d_testlab, d_trainwts,
- d_testwts;
+ rmm::device_uvector<float> d_train;
+ std::unique_ptr<rmm::device_uvector<float>> d_trainlab, d_test, d_testlab, d_trainwts, d_testwts;
};
TEST_F(GeneticEvolutionTest, SymReg)
@@ -264,7 +264,7 @@ TEST_F(GeneticEvolutionTest, SymReg)
cudaEventRecord(start, stream);
symFit(handle,
- d_train->data(),
+ d_train.data(),
d_trainlab->data(),
d_trainwts->data(),
n_tr_rows,
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.
Okay, I got what you meant. Thanks tamas!
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.
ah, I had just implemented what you meant in the latest commit tamas (jinx!)
cpp/test/sg/genetic/program_test.cu
Outdated
node* d_nodes1; | ||
node* d_nodes2; | ||
program_t d_progs; | ||
std::unique_ptr<rmm::device_uvector<float>> d_data, d_y, d_lYpred, d_lY, d_lunitW, d_lW, dx2, dy2, |
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.
Same as above, you do not need to wrap this in unique_ptr.
@teju85 not sure, haven't seen a timeout since increasing CI resources and a few ivfpq test modifications, let me rerun tests and if it's persistent we can push a fix |
rerun tests |
rerun tests |
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.
Thanks Venkat for the update! It looks good to me.
Note: I have only reviewed the C++ test.
rerun tests |
1 similar comment
rerun tests |
CI is timing out on the doxygen docs check, looking into it |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #3638 +/- ##
===============================================
Coverage ? 86.03%
===============================================
Files ? 231
Lines ? 18751
Branches ? 0
===============================================
Hits ? 16132
Misses ? 2619
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@gpucibot merge |
This PR contains the implementation of the core algorithms of gplearn(tournaments + mutations + program evaluations) in cuml. Tagging all involved: @teju85 @venkywonka @vinaydes The goal is to complete the following tasks: - [x] Implement program execution and metric evaluation for a given dataset on the GPU - [x] Implement a batched version of the above for all programs in a generation - [x] Run tournaments for program selection on the GPU - [x] Perform all mutations on the CPU - [x] Fit, Predict and Transform functions for api - [x] Tests for all individual functions - [x] Add an example demonstrating how to perform symbolic regression (a similar approach can be taken for transformation too) Authors: - Vimarsh Sathia (https://github.com/vimarsh6739) - Venkat (https://github.com/venkywonka) Approvers: - Robert Maynard (https://github.com/robertmaynard) - Venkat (https://github.com/venkywonka) - Thejaswi. N. S (https://github.com/teju85) - Corey J. Nolet (https://github.com/cjnolet) - Tamas Bela Feher (https://github.com/tfeher) - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#3638
This PR contains the implementation of the core algorithms of gplearn(tournaments + mutations + program evaluations) in cuml.
Tagging all involved: @teju85 @venkywonka @vinaydes
The goal is to complete the following tasks: