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

first metric only in earlystopping for cli #2172

Merged
merged 4 commits into from
May 16, 2019
Merged

Conversation

guolinke
Copy link
Collaborator

@StrikerRUS
Copy link
Collaborator

I added the note about that this param is for CLI only and slightly paraphrased description for the consistency with others.

@@ -260,7 +260,8 @@ struct Config {
// desc = ``<= 0`` means disable
int early_stopping_round = 0;

// desc = will only use first metric for early stopping if set to ``True``
// desc = set this to ``true``, if you want to use only the first metric for early stopping
// desc = **Note**: can be used only in CLI version
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although this parameter is actually used in cli version, there is another parameter with the same name in python/R package. Therefore, I think the note is not needed, as well as the early_stopping_round.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little bit another situation (in Python, at least).
early_stopping_round is a parameter for train/cv, so everything is OK, but first_metric_only is a parameter for callback.

def early_stopping(stopping_rounds, first_metric_only=False, verbose=True):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay, I think the first_metric_only should be in train/cv as well, like the early_stopping_round:

if early_stopping_rounds is not None:
callbacks.add(callback.early_stopping(early_stopping_rounds, verbose=False))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think that we should try to retrieve first_metric_only from params and pass to callback?

if early_stopping_rounds is not None:
callbacks.add(callback.early_stopping(early_stopping_rounds, verbose=bool(verbose_eval)))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, just like the early stopping rounds:

for alias in ["early_stopping_round", "early_stopping_rounds", "early_stopping"]:
if alias in params and params[alias] is not None:
early_stopping_rounds = int(params.pop(alias))
warnings.warn("Found `{}` in params. Will use it instead of argument".format(alias))
break

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! Then I'll create a PR for this and remove my note.

@guolinke guolinke merged commit f01b2ac into master May 16, 2019
@StrikerRUS StrikerRUS deleted the cli_es_first_metric branch May 16, 2019 11:50
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants