Skip to content

Add skip_train option for metrics#662

Merged
arcadia-devtools merged 5 commits into
catboost:masterfrom
RakitinDen:skip_train
Feb 26, 2019
Merged

Add skip_train option for metrics#662
arcadia-devtools merged 5 commits into
catboost:masterfrom
RakitinDen:skip_train

Conversation

@RakitinDen

Copy link
Copy Markdown

Added skip_train options for metrics in cross validation.

I hereby agree to the terms of the CLA available at https://yandex.ru/legal/cla/?lang=en.

@@ -299,7 +303,6 @@ static void DisableMetricSkipTrain(NJson::TJsonValue* metric) {
}
hints["skip_train"] = "false";
params["hints"] = MakeHintsDescription(hints);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisableMetricSkipTrain is now not needed.

assert "test-AUC-mean" in results
return local_canonical_file(remove_time_from_json(JSON_LOG_PATH))

def test_cv_skip_train(task_type):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add test with AUC which has default skip_train=True and check that there is no train in results.

DisableMetricsSkipTrain(&updatedTrainOptionsJson);


// TODO(nikitxskv): Remove this hot-fix and make correct skip-metrics support in cv.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO should be removed

@annaveronika

Copy link
Copy Markdown
Contributor

Thanks a lot for the changes! There were some updates in code, could you resolve the conflicts?

@andrey-khropov

Copy link
Copy Markdown
Member

@arcadia-devtools Ship it!

@arcadia-devtools

Copy link
Copy Markdown
Collaborator

@andrey-khropov, internal review request created: 736861

@arcadia-devtools
arcadia-devtools merged commit 717d66b into catboost:master Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants