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

[Blocking][jvm-packages] fix the early stopping feature #3808

Merged
merged 13 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@CodingCat
Member

CodingCat commented Oct 18, 2018

added direction parameter and also fix the judge criterion

closes #3631

closes #3140

@CodingCat CodingCat requested a review from superbobry Oct 18, 2018

@CodingCat CodingCat changed the title from [jvm-packages] fix the early stopping feature to [Blocking][jvm-packages] fix the early stopping feature Oct 18, 2018

@CodingCat

This comment has been minimized.

Member

CodingCat commented Oct 18, 2018

also @yanboliang , can you take a look?

@hcho3 hcho3 referenced this pull request Oct 18, 2018

Closed

[ANNOUCEMENT] 0.81 release planned on November 1, 2018 #3744

12 of 12 tasks complete
if (numEarlyStoppingRounds > 0) {
if (!params.contains("metrics_expected_direction") &&
(params("metrics_expected_direction") == "asc" ||
params("metrics_expected_direction") == "desc")) {

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

Can you explain the logic here? I think we just need to check params.contains("metrics_expected_direction"). If users don't set metrics_expected_direction explicitly, then params doesn't contain it.

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

BTW, I saw there is param validation check at the definition:

final val metricsExpectedDirection = new Param[String](this, "metricsExpectedDirection",
    "the expected direction of the metrics' change, asc or desc",
    (value: String) => value == "asc" || value == "desc")

This will be called automatically with the two params setting method(MLlib way and XGBoost param map way), so inside the code, we don't need to validate params. I think here we just need to check params.contains("metrics_expected_direction"). If contained, it must be legal.

This comment has been minimized.

@CodingCat

This comment has been minimized.

@CodingCat

CodingCat Oct 22, 2018

Member

changed to boolean

if (params.get("metrics_expected_direction").equals("asc")) {
metricsExpectedDirection = true;
} else if (params.get("metrics_expected_direction").equals("desc")) {
metricsExpectedDirection = false;

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

The code here is a little confused. If we use boolean to represent asc or desc, it's better to rename metricsExpectedDirection as metricsExpectedAsc.

This comment has been minimized.

@CodingCat

CodingCat Oct 22, 2018

Member

updated

final val metricsExpectedDirection = new Param[String](this, "metricsExpectedDirection",
"the expected direction of the metrics' change, asc or desc",
(value: String) => value == "asc" || value == "desc")

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

It's better to make it case-insensitive via value.toLowerCase

This comment has been minimized.

@CodingCat

CodingCat Oct 22, 2018

Member

changed to boolean

@@ -118,7 +118,7 @@ public static Booster train(
* performance on the validation set.
* @param metrics array containing the evaluation metrics for each matrix in watches for each
* iteration
* @param earlyStoppingRound if non-zero, training would be stopped
* @param earlyStoppingRounds if non-zero, training would be stopped
* after a specified number of consecutive
* increases in any evaluation metric.

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

increases is out of date? It depends on metrics_expected_direction.

This comment has been minimized.

@CodingCat

CodingCat Oct 22, 2018

Member

updated

}
if (!decreasing) {
boolean onTrack = judgeIfTrainingOnTrack(params, earlyStoppingRounds, metrics, iter);

This comment has been minimized.

@yanboliang

yanboliang Oct 21, 2018

Contributor

Can we put this check inside of earlyStoppingRounds > 0? As if users don't set early stopping, we don't need to check it, that's should be more efficient.

This comment has been minimized.

@CodingCat

CodingCat Oct 22, 2018

Member

updated

@yanboliang

This comment has been minimized.

Contributor

yanboliang commented Oct 21, 2018

BTW, do we need to port metrics_expected_direction to python xgboost?

@hcho3

This comment has been minimized.

Collaborator

hcho3 commented Oct 21, 2018

@yanboliang Python XGBoost already has a list of metrics to be maximized:

maximize_metrics = ('auc', 'map', 'ndcg')
maximize_at_n_metrics = ('auc@', 'map@', 'ndcg@')
maximize_score = maximize
metric_label = env.evaluation_result_list[-1][0]
metric = metric_label.split('-', 1)[-1]
if any(metric.startswith(x) for x in maximize_at_n_metrics):
maximize_score = True
if any(metric.split(":")[0] == x for x in maximize_metrics):
maximize_score = True

@yanboliang

This comment has been minimized.

Contributor

yanboliang commented Oct 21, 2018

@hcho3 Thanks for pointing it out. Then what about renaming metrics_expected_direction to maximize_evaluation_metrics, with type of boolean? I think the later one should be easy to understand for users, and it's more closer to Python XGBoost. Also cc @CodingCat What do you think of?

@CodingCat

This comment has been minimized.

Member

CodingCat commented Oct 22, 2018

@yanboliang thanks for the review, addressed the comments

@deepxg

This comment has been minimized.

deepxg commented Oct 23, 2018

Can I suggest that we make early stopping a policy-based API with some sort of callback? So instead of just a count and a direction, we supply an object which receives the metrics up to the current round, and whatever other information might be useful, and we have a default implementation that is just the normal early stopping after X rounds without increase/decrease. I ask because I suspect there might be circumstances where you care about more than just an absolute number of rounds, or you might for efficiency reasons want to stop training even while still decreasing, just because the returns appear to be diminishing. Externalising the decision and making it configurable would future-proof the API somewhat.

So, just something like IStopping with boolean shouldStopAfter(float[][] metrics) in the simplest case, but arguments could be made for more context. Making these objects stateful across training runs also simplifies the logic in the early stopping case, as you can just maintain the min/max as you go, and a count of how long ago it was.

@CodingCat

This comment has been minimized.

Member

CodingCat commented Oct 23, 2018

@deepxg it's a good suggestion. However, I think it is out of the scope of this PR:

  1. 0.81 (the coming version) is a minor release version so it should not have break changes of previous API

  2. and the current implementation is to keep consistent with all other language bindings...

we might evaluate and work on your suggestion in a more comprehensive way which covers all language bindings in the coming releases

@CodingCat

This comment has been minimized.

Member

CodingCat commented Oct 23, 2018

@yanboliang ping for review?

@yanboliang

LGTM

@CodingCat CodingCat merged commit 4ae225a into dmlc:master Oct 23, 2018

5 checks passed

Jenkins: Build & Test Stage built successfully
Details
Jenkins: Get sources Stage built successfully
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@CodingCat CodingCat deleted the CodingCat:early_stopping branch Oct 23, 2018

@yanboliang

This comment has been minimized.

Contributor

yanboliang commented Oct 23, 2018

I think @deepxg have a good point, we should consider it in the roadmap. Thanks.

CodingCat added a commit to CodingCat/xgboost that referenced this pull request Oct 25, 2018

[Blocking][jvm-packages] fix the early stopping feature (dmlc#3808)
* add back train method but mark as deprecated

* add back train method but mark as deprecated

* add back train method but mark as deprecated

* add back train method but mark as deprecated

* fix scalastyle error

* fix scalastyle error

* fix scalastyle error

* fix scalastyle error

* temp

* add method for classifier and regressor

* update tutorial

* address the comments

* update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment