8000 [MRG] DEP deprecate "mae" criterion in GadientBoosting estimators by madhuracj · Pull Request #18326 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] DEP deprecate "mae" criterion in GadientBoosting estimators #18326

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 4, 2020
Merged

[MRG] DEP deprecate "mae" criterion in GadientBoosting estimators #18326

merged 4 commits into from
Sep 4, 2020

Conversation

madhuracj
Copy link
Contributor

…oostingRegressor

Reference Issues/PRs

Fixed #18263

What does this implement/fix? Explain your changes.

Deprecate criterion='mae' in GradientBoostingClassifier and GradientBoostingRegressor

Any other comments?

NicolasHug
NicolasHug reviewed Sep 2, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

thanks @madhuracj, looks good. We'll also need a whatsnew entry.

Not sure why the docs aren't building...

@madhuracj
Copy link
Contributor Author

@NicolasHug Thanks for reviewing. Changelog entry added in f9f1a3e. Still the docs aren not building.

@alfaro96
Copy link
Member
alfaro96 commented Sep 3, 2020

Something related with CircleCI is failing for this PR (the lint job is not working neither). The other PRs are properly building the docs.

I think that we do not need to worry (so much) about this.

alfaro96
alfaro96 approved these changes Sep 3, 2020
View reviewed changes
Copy link
Member
@alfaro96 alfaro96 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 @madhuracj for your PR!

LGTM.

@glemaitre glemaitre changed the title [MRG] Deprecate criterion='mae' in GradientBoostingClassifier and GradientBoostingRegressor [MRG] DEP deprecate "mae" criterion in GadientBoosting estimators Sep 3, 2020
glemaitre
glemaitre approved these changes Sep 3, 2020
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @madhuracj

NicolasHug
NicolasHug approved these changes Sep 3, 2020
View reviewed changes
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @madhuracj .

To briefly explain the reason, let's add those at the end of each warning / doc message:

  • for the regressor: "The correct way of minimizing the absolute error is to use loss='lad' instead."
  • for the classifier: "Use criterion='friedman_mse' or 'mse' instead, as trees should use a least-square criterion in Gradient Boosting."

@madhuracj
Copy link
Contributor Author

LGTM, thanks @madhuracj .

To briefly explain the reason, let's add those at the end of each warning / doc message:

  • for the regressor: "The correct way of minimizing the absolute error is to use loss='lad' instead."
  • for the classifier: "Use criterion='friedman_mse' or 'mse' instead, as trees should use a least-square criterion in Gradient Boosting."

Done in e04af1c.
8000

@NicolasHug
Copy link
Member

In the warnings as well please ;)

@madhuracj
Copy link
Contributor Author

Sorry, I missed that part. Hopefully, 7d63e66 fulfils that.

@glemaitre glemaitre merged commit 4de0f97 into scikit-learn:master Sep 4, 2020
@glemaitre
Copy link
Member

Thanks @madhuracj

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@madhuracj @jayzed82
DEP deprecate "mae" criterion in GadientBoosting estimators (scikit-l…
f91596c 
…earn#18326)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug NicolasHug approved these changes

@glemaitre glemaitre glemaitre approved these changes

@alfaro96 alfaro96 alfaro96 approved these changes

Assignees
No one assigned
Labels
module:ensemble
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't allow criterion='mae' for gradient boosting estimators
4 participants
@madhuracj @alfaro96 @NicolasHug @glemaitre
0