DEP deviance in favor of log_loss for GradientBoostingClassifier #23036
Conversation
sklearn/ensemble/_gb.py
Outdated
| boosting recovers the AdaBoost algorithm. | ||
| loss : {'log_loss', 'exponential'}, default='log_loss' | ||
| The loss function to be optimized. 'log_loss' refers to binomial and | ||
| multinomial deviance, the same as used in linear logistic regression. |
There was a problem hiding this comment.
To be consistent with the docstring change above this one, can we remove the reference to "deviance"?
| multinomial deviance, the same as used in linear logistic regression. | |
| multinomial log loss, the same as used in logistic regression. |
(Also I think most think of logistic regression as linear model.)
There was a problem hiding this comment.
Hmm. How about calling them just binomial and multinomial log-likelihood?
There was a problem hiding this comment.
"binomial" and "multinomial" are distributions, have a likelihood and, equivalently, a deviance. Log loss, although derivable from those likelihoods, is a loss that a priori has no distribution. I would speak of binary and multiclass log loss.
sklearn/ensemble/_gb.py
Outdated
| deviance (= logistic regression) for classification | ||
| with probabilistic outputs. For loss 'exponential' gradient | ||
| boosting recovers the AdaBoost algorithm. | ||
| loss : {'log_loss', 'exponential'}, default='log_loss' |
There was a problem hiding this comment.
During the deprecation, "deviance" is still valid and should stay in the set of valid losses
There was a problem hiding this comment.
I have seen both. E.g. the deprecation of the "mae" and "mse" criterion in trees are also not listed anymore during the deprecation cycle. They are only listed under deprecated.
I personally prefer it this way, but will do as reviewers wish.
There was a problem hiding this comment.
There's also the default value that is directly changed. The thing is that it's just a renaming that gives the same model, so maybe it's acceptable. I'm not sure if it can be a problem ?
There was a problem hiding this comment.
I personally prefer it this way, but will do as reviewers wish.
I don't have a strong opinion, but I'd like that we always do it the same way (I didn't remember that we did it this way in the trees). Maybe we should explain how to update the doc in the contributing guide.
There was a problem hiding this comment.
So I re-introduce them.
There was a problem hiding this comment.
LGTM.
There's just one thing I'm not sure about. Here the default changes from "deviance" to "log_loss" immediately. Usually when we rename, we make the change effective at the end of the cycle. Here it should be fine since both are equivalent so I think we can safely move forward, but maybe I'm missing something ?
|
@jeremiedbb Thanks for finishing. |
…kit-learn#23036) Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Reference Issues/PRs
Partially addresses #18248
What does this implement/fix? Explain your changes.
This PR deprecates
loss="deviance"in favor ofloss="log_loss"inGradientBoostingClassifier. The default is changed accordingly.