Skip to content

MAINT Deprecate scoring='max_error' and replace it by scoring='neg_max_error'#29462

Merged
adrinjalali merged 10 commits intoscikit-learn:mainfrom
artificialfintelligence:issue/13887
Jul 19, 2024
Merged

MAINT Deprecate scoring='max_error' and replace it by scoring='neg_max_error'#29462
adrinjalali merged 10 commits intoscikit-learn:mainfrom
artificialfintelligence:issue/13887

Conversation

@artificialfintelligence
Copy link
Copy Markdown
Contributor

@artificialfintelligence artificialfintelligence commented Jul 11, 2024

Reference Issues/PRs

Fixes #29417

This deprecates scoring='max_error and replaces it with scoring='neg_max_error'.

This is very similar to #14898 that did a similar thing for scoring='brier_score_loss' => scoring='neg_brier_score'

What does this implement/fix? Explain your changes.

Renames the max_error scorer to neg_max_error in order to make it consistent with other scorers that have greater_is_better = False (and consistent with the documentation as well). Deprecates max_error with a deprecation warning message stating that it will be removed in v1.8.

Any other comments?

Please remove the deprecation warning, deprecation test case and the two comment lines I added (which start with XXX) once the old scorer is removed.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 11, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 0e53cce. Link to the linter CI: here

@lesteve lesteve changed the title Issue/13887 MAINT Deprecate scoring='max_error' and replace it by scoring='neg_max_error' Jul 11, 2024
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jul 11, 2024

Thanks for your PR, I edited your description and PR title to help potential reviewers!

Comment thread doc/modules/model_evaluation.rst
Comment thread sklearn/metrics/_scorer.py Outdated
Comment thread sklearn/metrics/_scorer.py Outdated
Comment thread sklearn/metrics/_scorer.py Outdated
Copy link
Copy Markdown
Member

@lesteve lesteve 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!

Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

LGTM, and it seems consistent with the other work we've done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation section 3.4.1.1 has incorrect description that would be correct if the max_loss metric were to be tweaked and renamed

3 participants