-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[jvm-packages] clean up example #10618
Conversation
Hi @trivialfis @hcho3, please help review this PR which is the first small PR of #10617, thx |
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.
Some questions regarding the examples, rest looks good.
@@ -48,6 +49,6 @@ object BoostFromPrediction { | |||
testMat.setBaseMargin(testPred) | |||
|
|||
System.out.println("result of running from initial prediction") | |||
val booster2 = XGBoost.train(trainMat, params.toMap, 1, watches.toMap, null, null) | |||
XGBoost.train(trainMat, params.toMap, 1, watches.toMap, null, null) |
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.
What is this example doing if it's not showing the result?
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.
Guess it's just trying to verify if the training is ok.
@@ -40,7 +40,6 @@ object CrossValidation { | |||
// set additional eval_metrics | |||
val metrics: Array[String] = null | |||
|
|||
val evalHist: Array[String] = | |||
XGBoost.crossValidation(trainMat, params.toMap, round, nfold, metrics) | |||
XGBoost.crossValidation(trainMat, params.toMap, round, nfold, metrics) |
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.
Since this is an example, maybe it's useful to print it?
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 would not to refine the tests in this PR. Maybe have the follow up after merging my all small PRs for reworking jvm
This is the 1st PR of #10617