Skip to content
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

[SPARK-7770] [ML] GBT validationTol change to compare with relative or absolute error #8549

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
3 participants
@yanboliang
Copy link
Contributor

commented Sep 1, 2015

GBT compare ValidateError with tolerance switching between relative and absolute ones, where the former one is relative to the current loss on the training set.

@SparkQA

This comment has been minimized.

Copy link

commented Sep 1, 2015

Test build #41866 has finished for PR 8549 at commit 47bb5e6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@jkbradley

This comment has been minimized.

Copy link
Member

commented Sep 16, 2015

Btw, I commented on the JIRA. Let me know what you think.

@yanboliang yanboliang changed the title [SPARK-7770] [ML] GBT validationTol change to relative tolerance [SPARK-7770] [ML] GBT validationTol change to compare switch between relative and absolute error Sep 18, 2015

@yanboliang yanboliang changed the title [SPARK-7770] [ML] GBT validationTol change to compare switch between relative and absolute error [SPARK-7770] [ML] GBT validationTol change to compare with relative or absolute error Sep 18, 2015

@SparkQA

This comment has been minimized.

Copy link

commented Sep 18, 2015

Test build #42655 has finished for PR 8549 at commit 9790bb0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@SparkQA

This comment has been minimized.

Copy link

commented Sep 18, 2015

Test build #42656 has finished for PR 8549 at commit ffea283.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@jkbradley

This comment has been minimized.

Copy link
Member

commented Sep 22, 2015

This looks good, but can you please document the semantics of validationTol in BoostingStrategy?

@SparkQA

This comment has been minimized.

Copy link

commented Sep 23, 2015

Test build #42879 has finished for PR 8549 at commit e6cf283.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@@ -262,7 +263,8 @@ object GradientBoostedTrees extends Logging {
validationInput, validatePredError, baseLearnerWeights(m), baseLearners(m), loss)
validatePredErrorCheckpointer.update(validatePredError)
val currentValidateError = validatePredError.values.mean()
if (bestValidateError - currentValidateError < validationTol) {
if (bestValidateError - currentValidateError < validationTol * Math.max(
currentPredError, 0.01)) {

This comment has been minimized.

Copy link
@jkbradley

jkbradley Sep 23, 2015

Member

I realized that it may make more sense to use currentValidateError instead of currentPredError here. That's what users would expect from "relative tolerance." Could you please update this + the doc? Thanks.

@SparkQA

This comment has been minimized.

Copy link

commented Sep 28, 2015

Test build #43058 has finished for PR 8549 at commit a9ef902.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@@ -250,7 +250,8 @@ object GradientBoostedTrees extends Logging {
predError = GradientBoostedTreesModel.updatePredictionError(
input, predError, baseLearnerWeights(m), baseLearners(m), loss)
predErrorCheckpointer.update(predError)
logDebug("error of gbt = " + predError.values.mean())
val currentPredError = predError.values.mean()

This comment has been minimized.

Copy link
@jkbradley

jkbradley Oct 2, 2015

Member

Since this is only used in logDebug, it would be better to put it inside logDebug so that it is not computed if debug logging is turned off.

@SparkQA

This comment has been minimized.

Copy link

commented Oct 8, 2015

Test build #43393 has finished for PR 8549 at commit 589180f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@jkbradley

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

LGTM, merging with master. Thanks for the PR!

@asfgit asfgit closed this in 2268356 Oct 8, 2015

@yanboliang yanboliang deleted the yanboliang:spark-7770 branch Oct 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.