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
[MRG] Fixed gradient computation in gradient boosting for multiclass classification #12715
[MRG] Fixed gradient computation in gradient boosting for multiclass classification #12715
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Indeed this PR seems to fix a bug, however it's probably not easy to write a non-regression test as the effect is probably very subtle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth throwing into 0.20.X?
It's hard to tell how strong the impact of the changes would be... My gut feeling is that it would be pretty minimal. |
I think nobody will notice the bug in practice. Not worth the backport a priori. But @jnothman feel free to do the backport if you disagree. |
Thanks @NicolasHug |
…assification (scikit-learn#12715) * Fixed gradient update * Added whatsnew entry
…assification (scikit-learn#12715) * Fixed gradient update * Added whatsnew entry
…class classification (scikit-learn#12715)" This reverts commit df30aa9.
…class classification (scikit-learn#12715)" This reverts commit df30aa9.
…assification (scikit-learn#12715) * Fixed gradient update * Added whatsnew entry
Reference Issues/PRs
Fixes #12696
What does this implement/fix? Explain your changes.
The gradients should be evaluated at iteration
i - 1
, that is, using the values ofy_pred
at iterationi - 1
In the current implementation,
y_pred
is partially updated during the loop over theK
trees of the current iteration. This means that the gradients aren't evaluated at iterationi - 1
anymore. It's a mix between iterationi - 1
and iterationi
.The current fix copies
y_pred
before looping over the trees to avoid this.I'm not sure how to propose a non-regression test for this.
Any other comments?