[python][sklearn] respect objective aliases

[StrikerRUS]

I believe this is quite odd to add objective alias used in sklearn (loss #4637) and simply ignore any objective aliases at the same time.

[StrikerRUS]

I believe this behavior is controversial and I'd better remove such silent replacement of objective function and instead raise error.

[shiyu1994]

I agree. We should explicitly tell the user what they intended to use is not valid. Or at least we should give a warning.

[jameslamb]

Totally support these changes and agree that this makes the scikit-learn API behave in a way that's closer to what we've told users to expect (where parameter aliases can always be used to override keyword arguments).

I just left one minor suggestion about some of the unrelated whitespace changes.

[shiyu1994]

Is this sentence complete? Should it be Will use it instead of argument `objective` ?

[StrikerRUS]

In this particular case I agree with you. Warning message was improved in 2d0392e.

However, please note that in this similar warning

LightGBM/python-package/lightgbm/engine.py

Lines 571 to 574 in da98f24

	for alias in _ConfigAliases.get("num_iterations"):
	if alias in params:
	_log_warning(f"Found `{alias}` in params. Will use it instead of argument")
	num_boost_round = params.pop(alias)

we cannot add specific argument name which is being overwritten because it depends on whether user uses train() function directly or indirectly from the sklearn-wrapper. Please consider the following example:

import lightgbm as lgb

from sklearn.datasets import load_boston

X, y = load_boston(return_X_y=True)
lgb_train = lgb.Dataset(X, y)

lgb.train({'n_iter': 5}, lgb_train)  # here n_iter is used instead of num_boost_round argument

lgb.LGBMRegressor(n_iter=5).fit(X, y)  # here n_iter is used instead of n_estimators argument

[shiyu1994]

I agree. We should explicitly tell the user what they intended to use is not valid. Or at least we should give a warning.

[StrikerRUS]

Should we block merging of this PR for one week similarly to #4580 (comment)?

[jameslamb]

Should we block merging of this PR for one week similarly to #4580 (comment)?

I support not merging this for a few more days, yes.

[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.