ENH Consistent loss name for absolute error #19733
Conversation
There was a problem hiding this comment.
Thank you for continuing this work @lorentzenchr !
Mostly looks good. I think we still need to update BaseEnsemble._make_estimator similar to what we did here:
scikit-learn/sklearn/ensemble/_base.py
Lines 158 to 161 in 3e45aee
sklearn/ensemble/_hist_gradient_boosting/tests/test_gradient_boosting.py
Outdated
Show resolved
Hide resolved
| @@ -194,26 +194,26 @@ def test_should_stop(scores, n_iter_no_change, tol, stopping): | |||
| assert gbdt._should_stop(scores) == stopping | |||
|
|
|||
|
|
|||
| def test_least_absolute_deviation(): | |||
| def test_absolute_error(): | |||
There was a problem hiding this comment.
For deprecations, we normally keep the original code and then configure the test to ignore the FutureWarnings. This is to make sure that we still support the original behavior during the deprecation period.
# TODO: Remove absolute_error in 1.2 when it is removed
@pytest.mark.filterwarnings("ignore::FutureWarning")
@pytest.mark.parametrize("loss", ["least_absolute_deviation", "absolute_error"])
def test_absolute_error(loss):For this specific case, there is an argument for just replacing the name, since it is a simple name change and adding another loss that does exactly the same thing would increase the runtime of tests.
I am +0.25 with replacing the name directly in the tests as you already done in this PR.
There was a problem hiding this comment.
Good to know for future deprecations, although it's not my intend to become a deprecation master:smirk:
Equivalence of new and old models' predict is always tested in test_loss_deprecated. So how is your rounding policy (and where is the 0.75 going to)?
Thanks for your support on this little project!
There was a problem hiding this comment.
So how is your rounding policy (and where is the 0.75 going to)?
It's more of an "absence of 0.75". Thinking about this again, I am now at +0.75 🤣.
| @@ -194,26 +194,26 @@ def test_should_stop(scores, n_iter_no_change, tol, stopping): | |||
| assert gbdt._should_stop(scores) == stopping | |||
|
|
|||
|
|
|||
| def test_least_absolute_deviation(): | |||
| def test_absolute_error(): | |||
There was a problem hiding this comment.
So how is your rounding policy (and where is the 0.75 going to)?
It's more of an "absence of 0.75". Thinking about this again, I am now at +0.75 🤣.
There was a problem hiding this comment.
Thanks @lorentzenchr ! LGTM, the merge conflict would need resolving.
@rth Done. |
|
The single CI failure is because the test took too long. I think it's safe to merge. Let me know if you prefer me to trigger CI again. |
|
@rth @thomasjpfan This has 2 approvals, CI all green. Someone might merge? |
|
Thanks @lorentzenchr ! |
Reference Issues/PRs
Partially solves #18248.
What does this implement/fix? Explain your changes.
This PR renames all variations of absolute error to
"absolute_error".