-
Notifications
You must be signed in to change notification settings - Fork 550
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
Rewrite random forest gtests #4038
Changes from all commits
812d9c7
2ac77c3
735c983
b077cf3
1f88053
af0aede
6f202ce
7ce4c66
c26d631
c990473
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
#include <locale> | ||
#include <map> | ||
#include <numeric> | ||
#include <raft/handle.hpp> | ||
#include <raft/mr/device/allocator.hpp> | ||
#include <raft/mr/host/allocator.hpp> | ||
#include <random> | ||
|
@@ -289,11 +290,6 @@ class DecisionTree { | |
(std::numeric_limits<L>::is_integer) ? CRITERION::ENTROPY : CRITERION::MSE; | ||
|
||
validity_check(tree_params); | ||
if (tree_params.n_bins > n_sampled_rows) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this check removed? Is it moved to some place else There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not correct because the quantiles have already been computed at this stage, checked with @venkywonka on this. I don't necessarily see any reason to enforce nbins < nrows. |
||
CUML_LOG_WARN("Calling with number of bins > number of rows!"); | ||
CUML_LOG_WARN("Resetting n_bins to %d.", n_sampled_rows); | ||
tree_params.n_bins = n_sampled_rows; | ||
} | ||
|
||
if (tree_params.split_criterion == | ||
CRITERION::CRITERION_END) { // Set default to GINI (classification) or MSE (regression) | ||
|
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.
Do we expect the floating points values such as
prediction
etc, match exactly?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've disabled any checks like this for now, but in the future I don't think that's an unreasonable goal for classification.