-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added random_forest_regressor #3624
Added random_forest_regressor #3624
Conversation
Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:
Thank you again for your contributions! 👍 |
@shaojunjie0912 Thank you for informing, I forgot to change the type of prediction variable. The size_t is trying to store double, the result was the large integer value you were checking. If there are anymore errors inform me, I will try my best to fix them. |
Hey @zoq, I completed everything that a random forest regressor needs, if there is any need for modification or addition, ping me. Now I want to add an adaboost regressor which is another part of that issue, these are the things I will be implementing:
|
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.
Looking at the code, do you think it would be possible to add another Predict
function to the existing RandomForest
implementation?
namespace mlpack { | ||
|
||
/** | ||
* This class implements a random forest regressor |
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.
Can you add some more details here? You can look at https://github.com/mlpack/mlpack/blob/master/src/mlpack/methods/linear_regression/linear_regression.hpp#L25 as a reference.
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.
Would be great to have a description for the none trivial template parameters.
//Calculate r2 score for random forest regressor | ||
double rfrR2Score = 1 - arma::sum(square(Ytest-rfrPred))/arma::sum(square(Ytest-TestMean)); | ||
|
||
arma::Row<double> dtrPred; | ||
dtr.Predict(Xtest, dtrPred); | ||
//Calculate r2 score for decision tree regressor |
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.
//Calculate r2 score for random forest regressor | |
double rfrR2Score = 1 - arma::sum(square(Ytest-rfrPred))/arma::sum(square(Ytest-TestMean)); | |
arma::Row<double> dtrPred; | |
dtr.Predict(Xtest, dtrPred); | |
//Calculate r2 score for decision tree regressor | |
// Calculate r2 score for random forest regressor. | |
double rfrR2Score = 1 - arma::sum(square(Ytest-rfrPred))/arma::sum(square(Ytest-TestMean)); | |
arma::Row<double> dtrPred; | |
dtr.Predict(Xtest, dtrPred); | |
// Calculate r2 score for decision tree regressor. |
Minor style adjustments, to follow https://github.com/mlpack/mlpack/wiki/DesignGuidelines.
// Loading data. | ||
data::DatasetInfo info; | ||
arma::mat trainData, testData; | ||
arma::rowvec trainResponses, testResponses; | ||
LoadBostonHousingDataset(trainData, testData, trainResponses, testResponses, | ||
info); | ||
|
||
// Train a random forest. | ||
RandomForestRegressor<> rfr(trainData, info, trainResponses, 25 /* 25 trees */, 1, | ||
1e-7, 0, MultipleRandomDimensionSelect(4)); | ||
|
||
REQUIRE(rfr.NumTrees() == 25); | ||
|
||
rfr.Train(trainData, info, trainResponses, 20 /* 20 trees */, 1, 1e-7, 0, | ||
true /* warmStart */, MultipleRandomDimensionSelect(4)); | ||
|
||
REQUIRE(rfr.NumTrees() == 25 + 20); |
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.
Looks like the indentation is off here, see https://github.com/mlpack/mlpack/wiki/DesignGuidelines#line-length-and-wrapping for more details.
arma::Row<double> dtrPred; | ||
dtr.Predict(Xtest, dtrPred); | ||
//Calculate r2 score for decision tree regressor | ||
double dtrR2Score = 1 - arma::sum(square(Ytest-dtrPred))/arma::sum(square(Ytest-TestMean)); |
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.
double dtrR2Score = 1 - arma::sum(square(Ytest-dtrPred))/arma::sum(square(Ytest-TestMean)); | |
double dtrR2Score = 1 - arma::sum(square(Ytest-dtrPred)) / arma::sum(square(Ytest-TestMean)); |
Use space between operations, also all names should follow the naming scheme https://github.com/mlpack/mlpack/wiki/DesignGuidelines#naming-conventions.
What should be the functionality for this new predict function that is different from other? |
Hi~ Pls when can this branch be merged into the master branch? |
Should probably ask the reviewer. |
Hey @zoq, when can this be merged? |
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
@arthiondaena sorry for our slow response on this, your PR just went stale so I have reopened it. |
Sure, I also need a review on the adaboost regressor pull request. |
@arthiondaena okay for sure, I will give it a review in the next couple of days |
@shrit Is this PR good to be merged or are there any changes required ? |
addresses issue #3598
I implemented random_forest_regressor class.