FEA add poisson loss to MLPRegressor - #30712
Conversation
| # TODO: Decide what to do with the term `xlogy(y_true, y_true) - y_true`. For now, | ||
| # it is included. But the _loss module doesn't use it (for performance reasons) and | ||
| # only adds it as return of constant_to_optimal_zero (mainly for testing). |
There was a problem hiding this comment.
This also needs a decision.
There was a problem hiding this comment.
But the _loss module doesn't use it (for performance reasons)
Is there any real performance impact of computing this term when training a model?
I think I would favor always including it. If it used for monitoring, displaying something named "Poisson loss" which is actually not the true loss, but the loss with a constant offset can be quite surprising.
There was a problem hiding this comment.
Alternatively, we could just rename this function to offset_poisson_loss to make it explicit that the computation of the constant terms are removed.
But if we expose the value of the objective function under the name "Poisson loss" via the public API, one way or another, I think we should make sure we re-add the constant terms to avoid any confusion when comparing values on resampled data (e.g. between train and test split to check for overfitting).
There was a problem hiding this comment.
I don't think performence is too much of a concern with this module so we might as well keep the constant terms.
There was a problem hiding this comment.
I don't think optimization is too much of a concern with this module so we might as well keep the constant terms.
The constant term in the loss only matters if we report/save those loss values as we do in MLP (and in GradientBoosting, but not in HistGradientBoosting). The nice thing about adding the constant term is that the minimum of the loss is then 0 exactly. Omitting it saves a bit of time.
Note also that NN libraries like pytorch usually omit the constant term (see https://pytorch.org/docs/stable/generated/torch.nn.PoissonNLLLoss.html#torch.nn.PoissonNLLLoss).
There was a problem hiding this comment.
Yes Tensorflow also omits it https://www.tensorflow.org/api_docs/python/tf/keras/losses/Poisson.
I would say let's keep it for now. Would we accomplish much by saving time in this module when there are other optimal libraries specifically for implementing neural networks?
7f517bb to
fb879c3
Compare
|
From my opinion including this seems to make sense and from an overall look at the changes I don't think it adds much extra maintenance overhead as well. What do others think @scikit-learn/core-devs |
|
In the monthly meeting on January 27, 2025, https://github.com/scikit-learn/administrative/blob/master/monthly_meetings/2025-01-27.md, we decided:
|
Is deprecating this module being considered as a possibility? Because if that is the case, I don't think we should add anything. |
|
The consensus was keep the module and make it consistent with the rest. Therefore add those few extensions, also the Poisson loss. Nobody has so far raised the voice to deprecate it. |
That seems fine. Thanks for explaining. I think we should move forward with reviewing this PR then. |
OmarManzoor
left a comment
There was a problem hiding this comment.
Thanks for the PR @lorentzenchr
A few comments otherwise looks nice
| # TODO: Decide what to do with the term `xlogy(y_true, y_true) - y_true`. For now, | ||
| # it is included. But the _loss module doesn't use it (for performance reasons) and | ||
| # only adds it as return of constant_to_optimal_zero (mainly for testing). |
There was a problem hiding this comment.
I don't think performence is too much of a concern with this module so we might as well keep the constant terms.
test_mlp_vs_poisson_glm_equivalent
ogrisel
left a comment
There was a problem hiding this comment.
Besides https://github.com/scikit-learn/scikit-learn/pull/30712/files#r1954012957 and getting the tests to pass with all admissible random seeds, LGTM.
test_mlp_vs_poisson_glm_equivalent
|
@ogrisel @OmarManzoor Thanks for your reviews. |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Omar Salman <omar.salman2007@gmail.com>
Reference Issues/PRs
None
What does this implement/fix? Explain your changes.
This adds the parameter
losstoMLPRegressor, same as for HGBT.Any other comments?
While in the past it was decided to not add any features to the neural nets, see https://scikit-learn.org/dev/faq.html#id9
I argue that it is important to support for all main model types (linear, tree-based, neural nets) the 3 most important distributions in statistics as losses:
We have it for all but the neural nets. Therefore this PR.