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

[python-package] allow custom weighing in fobj for scikit-learn API (closes #5027) #5211

Merged
merged 3 commits into from
Jun 27, 2022

Conversation

jmoralez
Copy link
Collaborator

This removes the automatic weighing for grad and hess when using a custom objective in the scikit-learn API and makes the signature equivalent to the custom eval (y_true, y_pred, weight, group).

Also adds tests to check that the behavior using the built-in objectives and the custom ones is consistent, i.e. if we give the weights to the dataset/fit using the built-in objectives we get the same results as if we use custom objectives and do the weighing ourselves.

Closes #5027.

@jmoralez jmoralez added the fix label May 13, 2022
@jmoralez jmoralez changed the title [python-package] allow custom weighing in fobj for scikit-learn API [python-package] allow custom weighing in fobj for scikit-learn API (closes #5027) May 13, 2022
@jmoralez jmoralez added breaking and removed fix labels May 13, 2022
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thank you so much for picking up this issue!
Please consider checking some my minor comments below:

python-package/lightgbm/sklearn.py Show resolved Hide resolved
python-package/lightgbm/sklearn.py Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
tests/python_package_test/test_sklearn.py Outdated Show resolved Hide resolved
@jmoralez jmoralez requested a review from StrikerRUS May 16, 2022 18:47
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Nice! Thank you very much!

@StrikerRUS
Copy link
Collaborator

@shiyu1994 Would you like to take a look at this PR as you were a creator of #5027?

@StrikerRUS
Copy link
Collaborator

@shiyu1994

1 similar comment
@StrikerRUS
Copy link
Collaborator

@shiyu1994

@StrikerRUS
Copy link
Collaborator

@jameslamb Please help with a second review.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for this! I believe I understand the changes, prior discussion on this PR, and previous discussion in #5207.

It's unfortunate that we have to make the breaking change where providing a callable that takes 3 arguments will now be treated as (y_true, y_pred, weight) when previous releases treated it as (y_true, y_pred, group), since the values in weight and group can be similar and therefore code that worked in lightgbm v3.3.2 could silently start producing incorrect results in v4.0.0.

But I agree that that's worth it in exchange for consistency with the existing interface for scikit-learn eval functions, as @StrikerRUS mentioned in

#5027 (comment)

Since v4.0.0 will contain many breaking changes intended to reduce inconsistencies, and since the project's current state doesn't really support doing a round of deprecation warnings first (e.g. #5133 (review)).

@StrikerRUS StrikerRUS merged commit b6deb9a into microsoft:master Jun 27, 2022
@jmoralez jmoralez deleted the custom-weighing branch June 27, 2022 23:43
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python] Weights is ignored when customized objective function is used
3 participants