Bug fix for first_metric_only on earlystopping

[matsuken92]

1. Order of metric list is not fixed even if it is defined by list. So explicitly indicating which metric is used on early stopping.
2. Due to introducing eval_train_metric feature, if the feature is enabled, then the first metric became train score of the metrics, so it does not appropriate for early stopping. So until the specified metrics of validation is coming, skipping to check early stopping in the loop.

[StrikerRUS]

Hi @matsuken92 !
Can you please rebase the branch and fix failing test to start reviewing process? Thanks!

[guolinke]

the tests failed. @matsuken92 @StrikerRUS any ideas?

[matsuken92]

The failure case of the test is that metric is default metric (None, "None" , "" and so on.) I'm now considering how to handle this case, so please wait.

[guolinke]

@matsuken92

Due to introducing eval_train_metric feature, if the feature is enabled, then the first metric became train score of the metrics, so it does not appropriate for early stopping. So until the specified metrics of validation is coming, skipping to check early stopping in the loop.

I think the first_metric_only should be used for all validation data (including training set when eval_train_metric is set). That is, the first metrics of all validation data should be considered in early stopping, not only the first metric of first validation data.

refer to #2049 (comment)

[StrikerRUS]

@guolinke Sorry, but I'm confused a little bit. Why need it to check early stopping on training data?
Current behavior is opposite:

If there is more than one, it will use all of them except the training data:

[guolinke]

@StrikerRUS training data will not meet early stopping condition, as it will be always better.
Therefore, using training data together with validation data for early stopping is okay.

[StrikerRUS]

@guolinke Then docs should be updated as well, right?
Refer to #1374 and #1393.
Or not?..

[guolinke]

@StrikerRUS we can keep them. although it actually checks the training metric...

[guolinke]

@matsuken92 any news ? the v2.2.4 release (#2138) is blocked by this PR.

[matsuken92]

Really sorry for my late response.
I will investigate it and commit the bug fix result on this week end.

[matsuken92]

Current status:

• Branch matsuken92:bugfix/first_metric_only is corrupted... Sorry I'm not familiar with git enough. So I created another branch bugfix/first_metric_only_train_metric based on latest version of microsoft/LightGBM master branch.
  https://github.com/matsuken92/LightGBM/tree/bugfix/first_metric_only_train_metric
• Now debugging. When metrics are specified on param metric, metrics, metric_types and so on, these cases are covered well. However, if metrics are specified on feval, difficult to handle them... The following notebook is my check script. https://github.com/matsuken92/LightGBM_test/blob/master/notebook/debug_for_metric_order.ipynb

[guolinke]

@matsuken92
Thanks! for the branch issue, you can force update bugfix/first_metric_only with the code of bugfix/first_metric_only_train_metric

[matsuken92]

@guolinke Thank you for your advice.
Now I fixed this issue on bugfix/first_metric_only_train_metric branch and add test items for them.
https://github.com/matsuken92/LightGBM/tree/bugfix/first_metric_only_train_metric
However, I still can not understand how to replace bugfix/first_metric_only_train_metric to bugfix/first_metric_only.
I would appreciate it if you would help me to replace them!

[StrikerRUS]

@matsuken92 Maybe you can create new PR for bugfix/first_metric_only_train_metric? I think it's the fastest and most painless way. This PR will be closed then.

[StrikerRUS]

Superseded by #2209.