-
Notifications
You must be signed in to change notification settings - Fork 525
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] Random Forest & Decision Tree Regression + major updates to Classification #635
Merged
dantegd
merged 58 commits into
rapidsai:branch-0.9
from
vishalmehta1991:fea-ext-randomforest_regression
Jul 1, 2019
Merged
Changes from 46 commits
Commits
Show all changes
58 commits
Select commit
Hold shift + click to select a range
e333239
Added rfRegressor class to random forest.
myrtw af36570
Added base dt class and DecisionTreeRegressor
myrtw 545582d
More class updates.
myrtw 8c5dd25
TreeNode updates & more dt class changes.
myrtw b9c5c6f
added regression kernels, modified naming convention to metric quest…
vishalmehta1991 04da29a
More decision tree changes
myrtw 14fb0e0
added kernels for mean squared error
vishalmehta1991 feaac6d
added all regression code / kernels, now compiles, next step is testing
vishalmehta1991 09eb479
Code in flux. Regression related changes.
myrtw 1b2b8d3
fixed right mse, it needs to be computed in kernel
vishalmehta1991 a6457fc
Added support for MSE or MAE split criterion.
myrtw d8176b7
Fixed split_criterion config in rf_test.
myrtw 229bd03
relocating functors, adding inline to device functors, adding entropy…
vishalmehta1991 f2a8336
Removed useless mem alloc+added tmp testing script
myrtw 76d2e7e
added iota and permute on GPU using thrust and ml-prims
vishalmehta1991 9a5f32c
Preprocess quantiles in batches.
myrtw b6ec0fb
merged new cuml dir structure
vishalmehta1991 b472bbd
Swapped cudaMemcpy w/ updateDevice/Host/Async.
myrtw d6027a9
Made rowids and colids unsigned int.
myrtw 29ac9ee
now using minmax primitive with column sampler
vishalmehta1991 8c5ed32
deleted col_minmax kernel now using ml-prims
vishalmehta1991 280bce6
adding missing stream in cub
vishalmehta1991 8648363
Reordered call to find_best_fruit_all function.
myrtw d48e9c2
Fixed nbins bug for GLOBAL_QUANTILE.
myrtw 21d30e9
Changelog update.
myrtw 6d374b3
removing unused function parameters and some comments
vishalmehta1991 507601d
adding label sampler in tree build
vishalmehta1991 76a5d3b
name change for gini/mse metric files
vishalmehta1991 c5abd26
Renamed dt class; sorted rf rowIDs; del MemGetInfo
myrtw 35f1b59
Moved plant, grow_tree methods to DecisionTreeBase class.
myrtw f8cb923
added depth zero helper function
vishalmehta1991 8417d37
Copied RF predictions on device. Added metrics too.
myrtw 895ccf6
moving allocations outside the loop
vishalmehta1991 5250be8
Added helper for tree fit.
myrtw e67b1f1
Merge remote-tracking branch 'origin/branch-0.8' into fea-ext-randomf…
vishalmehta1991 1dd63f4
Made RF's data input for predictions a GPU ptr.
myrtw babb624
Added unit-tests for Accuracy score.
myrtw 5f4a5fd
added support for when large number of features histograms do not fin…
vishalmehta1991 ae744ad
fixed missing plus sign
vishalmehta1991 fae84fe
Added unit-tests for regression metrics
myrtw dd7fa2d
Merge branch 'fea-ext-randomforest_regression' of github.com:vishalme…
vishalmehta1991 6f604cc
blocks max limit for classifier
vishalmehta1991 18ef5b2
Added support for wider datasets for minmax prim.
myrtw 4ab84a2
loop around regressor kernels for large number of features
vishalmehta1991 83d4dfb
Merge branch 'fea-ext-randomforest_regression' of github.com:vishalme…
vishalmehta1991 f79f8fa
Minor kernel fix + helper function.
myrtw e677e55
Merge branch 'branch-0.8' of github.com:rapidsai/cuml into fea-ext-ra…
myrtw 8d326b3
Python related updates to rf/dt, randomforest.pyx
myrtw b588047
WIP python wrapper changes.
myrtw 1181890
Fixed Python wrapper
myrtw 5907085
Merge branch 'branch-0.8' of github.com:rapidsai/cuml into fea-ext-ra…
myrtw 51017ac
adding host quatile data structure and removing host mem copies for n…
vishalmehta1991 10473d3
quantile fix; once per RF; quantile per tree flag; default false; tem…
vishalmehta1991 95eb5f1
critical fix: entropy functor cannot have log(0)
vishalmehta1991 9ad7e0f
Merge branch 'branch-0.9' of github.com:rapidsai/cuml into fea-ext-ra…
myrtw db787ef
Python style fix.
myrtw ffb80e2
using proper split criterion in rf test, fixing max value range for e…
vishalmehta1991 c6c5a5e
changing ChangeLog entry to branch-0.9
vishalmehta1991 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Copyright (c) 2019, NVIDIA CORPORATION. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#pragma once | ||
/* Return max. possible number of columns that can be processed within avail_shared_memory. | ||
Expects that requested_shared_memory is a multiple of ncols. */ | ||
int get_batch_cols_cnt(const size_t avail_shared_memory, const size_t requested_shared_memory, const int ncols) { | ||
int ncols_in_batch = ncols; | ||
int ncols_factor = requested_shared_memory / ncols; | ||
if (requested_shared_memory > avail_shared_memory) { | ||
ncols_in_batch = avail_shared_memory / ncols_factor; // floor div. | ||
} | ||
return ncols_in_batch; | ||
} | ||
|
||
|
||
/* Update batch_ncols (max. possible number of columns that can be processed within avail_shared_memory), | ||
blocks (for next kernel launch), and shmemsize (requested shared memory for next kernel launch). | ||
Precondition: requested_shared_memory is a multiple of ncols. */ | ||
void update_kernel_config(const size_t avail_shared_memory, const size_t requested_shared_memory, const int ncols, | ||
const int nrows, const int threads, int & batch_ncols, int & blocks, size_t & shmemsize) { | ||
batch_ncols = get_batch_cols_cnt(avail_shared_memory, requested_shared_memory, ncols); | ||
shmemsize = (requested_shared_memory / ncols) * batch_ncols; // requested_shared_memory is a multiple of ncols for all kernels | ||
blocks = min(MLCommon::ceildiv(batch_ncols * nrows, threads), 65536); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
There is one very interesting issue that just arose while @Salonijain27 was writing cython classes for Random Forest: this are not stateless APIs! This functions seem to just be an alternative way of calling the
DecisionTreeClassifier
(or equivalent RF class) Fit, but the object is still stateful! A stateless API means that the state is kept by the client (python) and the C++ code does not keep the state, in this case the C++ object still keeps the state in spite of there being afit
function outside of it.This not only complicates the cython work (not a deal breaker and it can be done to work) but also potentially complicates the pickling (model saving) functionality. For all the algorithms that are stateless (most of cuML) we automatically get that functionality since the python objects are the state of the algorithm. Wanted to start a quick conversation regarding this.
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.
(Of course feel free to correct any of this if the analysis/conclusions above are wrong!)
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.
Hmm.. that's interesting. IIUC, UMAP/kNN both expose such "stateful" classes, no? How are they being handled in cython world?
@cjnolet ^^^
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 they do, and the reason UMAP and KNN are the 2 classes that cannot be pickled currently (this was raised only a few weeks ago when some user ran into that issue). There is even an open issue #415 to create a stateless version of knn
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.
Also this is not necessarily something that needs to change per se, though it could make things much easier. We can look into using the
__getstate__
and__setstate__
python functions to implement the pickling. Will be looking into this today and the first days of next week. But even then the question of whether we want a uniformity in stateless(or fulness) of the C++ API still standsThere 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 noticed in the initial PR for the RF and DT algorithms that the stateless functions are being called by functions that maintain their own state and not the other way around. Unfortunately, as we were ramping up to release, I did not have the time to bring it up before it was merged. UMAP was implemented with a stateful convenience API built on top of a set of stateless functions and converting the cython to use those stateless functions will be trivial.
KNN was originally forced to be stateful because the FAISS Indexing API was designed in such a way that wrapping it with cython was non-trivial and the state was better off hidden from the cython layer altogether. Recent updates to their API have eliminated this problem and kNN will soon be reduced to a simple stateless
kneighbors
function on the C++ side. I'm also considering moving it to the prims.I do think we should continue to provide stateful convenience classes in the C++ layer. I would recommend, as a solution here, to expose a set of flat stateless functions, have the caller maintain the state, and also expose a stateful convenience API that is based on those stateless functions, maintaining the state for the users of the C++ API.
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.
We plan to implement a newer version of flat API for 0.9 release. The current rfRegressor API is inline to branch-0.8 rfClassifier