[python-package] early stopping min_delta (fixes #2526)

[jmoralez]

This aims to close #2526 by providing an additional argument min_delta to the early stopping callback, which allows to perform early stopping when the score doesn't improve by at least that min_delta in stopping_rounds iterations. The default value is 0, so the current behavior is maintained.

In the case for multiple metrics the user can provide a single delta for all metrics or a specific delta for each metric. The handling of all the cases (single metric, multiple metrics, multiple metrics considering first only) can be a bit cumbersome so I'm open to suggestions on how to handle them.

[jameslamb]

Thanks for working on this! I left a few preliminary comments.

[jameslamb]

@jmoralez by the way, since you just joined...we use a bot to create release notes automatically based on the set of pull requests merged between git tags.

That bot chooses which section of the release notes to put a change in based on the use of specific labels.

LightGBM/.github/release-drafter.yml

Lines 4 to 15 in 3942126

	- title: '💡 New Features'
	label: 'feature'
	- title: '🔨 Breaking'
	label: 'breaking'
	- title: '🚀 Efficiency Improvement'
	label: 'efficiency'
	- title: '🐛 Bug Fixes'
	label: 'fix'
	- title: '📖 Documentation'
	label: 'doc'
	- title: '🧰 Maintenance'
	label: 'maintenance'

So every PR needs to be assigned a label from that set. I'd call this one feature. If you're ever unsure, maintenance is a catch-all.

[StrikerRUS]

@jmoralez Wow, great idea of per metric thresholds, really like it! Do you have plans replicating the same logic in cpp code (PR for first_metric_only as a reference: #2172)?

[jmoralez]

I can give it a shot once this is done.

[StrikerRUS]

In general the proposed solution looks good to me. Thanks for the PR!

In dmlc/xgboost#7137 (comment) it is said that min_delta is a commonly used name for a such type of parameter. Maybe it is better to use name min_delta here as well for the consistency with other ML libraries?

Also, looking into APIs of early stopping callbacks from other frameworks (for example, pytorch-lightning and tensorflow), I see only one parameter min_delta and no tolerance parameter.

I believe all _log_warning() occurrences should be replaced with _log_info() because those messages look like debug-level info, not like something that is expected to be fixed at the user side.

Please enhance new parameter description with some info about the semantics of expected parameter types. It should help users avoid this error at the time of writing their code but not after they read this error message.

Must provide a single early stopping threshold or as many as metrics.

Something like the following:

    If float, this singe value is used as early stopping threshold for all metrics.
    If list, its length should match the total number of metrics to use one early stopping threshold per one metric.

[jmoralez]

@StrikerRUS I've changed the name to min_delta and updated the description. Let me know what you think.

[shiyu1994]

The changes LGTM. Thank you! @jmoralez

[StrikerRUS]

Sorry for the delay!

Everything is great, thanks a lot!

I just think we should allow users to silence debugging messages. Moreover, we already have verbose argument for this callback.

[StrikerRUS]

Thank you very much for implementing this feature! LGTM!

Just noticed that two my suggestions from the previous review were not so neat.

[jmoralez]

@StrikerRUS should I merge this?

[StrikerRUS]

@jmoralez

should I merge this?

TBH, I'm not sure. We decided to give some time (about 2 weeks or so) after the v3.3.1 release for reporting critical bugs in it and during that time not merge large PRs that contain new features.

I'd like to ask to not merge new large PRs with major features for one or two weeks after releasing v3.3.0. This will ease the process of creating v3.3.1 hotfix release if that will be needed in case of some critical bugs in v3.3.0 we might get reports about.
#4633 (comment)

I'd also like to propose that after the v3.3.1 release, we again wait 2 weeks to begin merging other large PRs, in case a v3.3.2 release is required.
#4715 (comment)

This PR touches existing code and may possibly change current logic. I'd better wait a little bit before merging it.
@jameslamb @shiyu1994 WDYT?

[jameslamb]

I'd better wait a little bit before merging it

I agree. v3.3.1 was only merged 6 days ago (#4715 (comment)), so I think we should wait another week for this PR.

[shiyu1994]

I'd better wait a little bit before merging it.

I agree.

[jameslamb]

Great work on this, @jmoralez !!!

[github-actions]

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.