[MRG+2] Add max_error to the existing set of metrics for regression #12232

whiletruelearn · 2018-10-01T10:40:42Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Added a new metric for regression which is max_error. It is the worst case error between the predicted value and the true value.

 >>> from sklearn.metrics import max_error
 >>> y_true = [3.1, 2.4, 7.6, 1.9]
 >>> y_pred = [4.1, 2.3, 7.4, 1.7]
 >>> max_error(y_true, y_pred)
    1.0

Any other comments?

eamanu

Please add some test

eamanu · 2018-10-01T12:55:44Z

Add [WIP] to the PR name

qinhanmin2014 · 2018-10-01T13:11:18Z

Is this metric widely used? Do you have some more references about the metric? Personally I'm not persuaded to add it.
If you add a metric, you also need a scorer.

whiletruelearn · 2018-10-01T13:43:27Z

@qinhanmin2014 I have updated scorer and It is cited as as a metric here
Thought it will be nice to have in sklearn as well.

qinhanmin2014 · 2018-10-01T14:34:44Z

@qinhanmin2014 I have updated scorer and It is cited as as a metric here
Thought it will be nice to have in sklearn as well.

I've read that in your issue but I don't think it's persuasive. Some papers/books/a wiki entry might be better from my side, or you can wait to see if other core devs will support you. We already have many metrics and I'm personally a bit selective for new metrics.

whiletruelearn · 2018-10-01T16:03:55Z

@qinhanmin2014 Thanks for the review. I am closing this PR.

Wanted to get your thoughts on this. I see bunch of other metrics related to regression.

MPE
MAPE
adjusted R2
AIC
BIC

Is there any historical reason why these metrics are not considered. Or would it be nice to have them implemented?. I see that Statsmodel currently prints out these metrics when we call summary on a model.

eamanu · 2018-10-01T16:03:56Z

I sincerely, heard about this very little. But, I will prefer to have more and not less.

qinhanmin2014 · 2018-10-02T03:02:51Z

I sincerely, heard about this very little. But, I will prefer to have more and not less.

@eamanu I'm not opposed to more metrics, my point here is that we should ensure that the metrics we included are well-defined and widely accepted, but none of you have provided sufficient evidence to show that this metric is widely accepted.

qinhanmin2014 · 2018-10-02T03:16:17Z

@qinhanmin2014 Thanks for the review. I am closing this PR.

@whiletruelearn Thanks for the PR. If you want more feedback from others, feel free to reopen it :)

Wanted to get your thoughts on this. I see bunch of other metrics related to regression.

MAPE is likely to be included in 0.21 (see #10711)
For other metrics, I'm unable to recall (and find) related issue/PR so if you have enough references to show that they are well-defined and widely accepted, I think contributions are welcomed.

jnothman · 2018-10-03T07:09:32Z

I get why this might be useful as a diagnostic measure. I think its meaning is unambiguous for single output regression. I wouldn't object to providing it with appropriate motivation in the user guide.

whiletruelearn · 2018-10-03T07:20:30Z

@jnothman can i reopen this PR and work on this ?

qinhanmin2014 · 2018-10-03T07:32:55Z

I wouldn't object to providing it with appropriate motivation in the user guide.

This is also what I want to know about the metric.

whiletruelearn · 2018-10-03T10:26:50Z

I get why this might be useful as a diagnostic measure. I think its meaning
is unambiguous for single output regression.

@jnothman I feel the same way. It is a simple diagnostic measure that will tell how good a fit the model is. It is straightforward to understand for everyone.

@qinhanmin2014 Sharing a bunch of links where people have discussed about this metric. Having dived a little deeper, i feel this metric can be useful.

[1] https://stats.stackexchange.com/questions/197642/linear-fit-how-to-minimize-maximum-error-rather-than-average-error
[2] https://math.stackexchange.com/questions/47944/linear-regression-for-minimizing-the-maximum-of-the-residuals
[3] https://lib.dr.iastate.edu/cgi/viewcontent.cgi?referer=https://www.google.co.in/&httpsredir=1&article=7550&context=rtd
[4] https://pdfs.semanticscholar.org/7971/868bf856e611aca547e01c29d6146f117450.pdf

Please let me know if i need something else to be changed

qinhanmin2014 · 2018-10-03T11:32:27Z

@jnothman I feel the same way. It is a simple diagnostic measure that will tell how good a fit the model is. It is straightforward to understand for everyone.

In my previous comments, I'm not arguing that this metric is not well-defined. I'm worrying about whether it's widely accepted.

@qinhanmin2014 Sharing a bunch of links where people have discussed about this metric. Having dived a little deeper, i feel this metric can be useful.

Thanks for these materials, but they are about a regression method which minimize maximum error, right? (apologies if I missed something in the 100+ page reference). I'd rather see something defining such a metric, though these seems enough to keep me +0 instead of -1 for it.

sklearn/metrics/regression.py

qinhanmin2014 · 2018-10-03T11:37:10Z

sklearn/metrics/regression.py

@@ -573,3 +574,37 @@ def r2_score(y_true, y_pred, sample_weight=None,
        avg_weights = multioutput

    return np.average(output_scores, weights=avg_weights)
+
+
+def max_error(y_true, y_pred):


I guess we can support sample_weight here, right?

Thought about this for some time . Does it make sense to add? I see that median_absolute_error also doesn't accept sample weight. We are calculating the max residual error right, i couldn't fully understand the purpose sample_weight would provide here.

See #3450 and #6217

Thanks. I have made the change. Can you please review.

qinhanmin2014 · 2018-10-03T11:38:04Z

sklearn/metrics/__init__.py

@@ -60,6 +60,7 @@
 from .regression import mean_squared_log_error
 from .regression import median_absolute_error
 from .regression import r2_score
+from .regression import max_error


alphabet order

qinhanmin2014 · 2018-10-03T11:42:47Z

sklearn/metrics/regression.py

+    1.0
+    """
+    y_type, y_true, y_pred, _ = _check_reg_targets(y_true, y_pred,
+                                                   'uniform_average')


This seems awkward. If we decide to include the metric, we might add a default value to multioutput

changed it to None. Is that the right approach?

eamanu · 2018-10-03T16:05:36Z

sklearn/metrics/regression.py

+                                                   'uniform_average')
+    if y_type == 'continuous-multioutput':
+        raise ValueError("Multioutput not supported in max_error")
+    max_error = np.around(np.max(np.abs(y_true - y_pred)), decimals=3)


I agree with @qinhanmin2014. Or why not create a parameter to set the decimal number?

whiletruelearn · 2018-10-04T08:14:54Z

@qinhanmin2014 sorry for not stating clearly earlier.
In [4] https://pdfs.semanticscholar.org/7971/868bf856e611aca547e01c29d6146f117450.pdf

section 2.3 speaks about this metric.

jnothman

Tbh I don't know much about regression evaluation either, but this intuitively seems reasonable to me. The question of whether it's inappropriate to maintain this along with all the others is hard to decide, when someone may indeed question sample_weight support, or multi-output regression...

qinhanmin2014

I'll follow Joel, @whiletruelearn please add something to the user guide.

qinhanmin2014 · 2018-10-06T12:32:32Z

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:.

qinhanmin2014 · 2018-10-06T12:33:01Z

At least it's straightforward and easy to maintain :)

eamanu · 2018-10-06T13:34:42Z

When you add the entry to the change log (@qinhanmin2014 comment), it will ready for me.

whiletruelearn · 2018-10-07T07:23:16Z

Thanks @jnothman @qinhanmin2014 @eamanu .

I have updated the change log as well.

qinhanmin2014 · 2018-10-07T07:33:10Z

please add something to the user guide.

whiletruelearn · 2018-10-07T07:38:32Z

@qinhanmin2014 do you mean over metrics.rst ?
I don't see any other metric for regression mentioned over there.

qinhanmin2014 · 2018-10-07T07:48:57Z

See http://scikit-learn.org/dev/modules/model_evaluation.html#regression-metrics

whiletruelearn · 2018-10-08T05:37:07Z

@qinhanmin2014 I have updated the user guide. Can you please let me know if this if fine ?

whiletruelearn · 2018-10-08T09:53:19Z

Thanks @jnothman . I have updated the docs based on your suggestion.

jnothman · 2018-10-08T13:27:22Z

doc/modules/model_evaluation.rst

+Max error
+-------------------
+
+The :func:`max_error` function computes the  maximum `residual error <https://en.wikipedia.org/wiki/Errors_and_residuals`_,


please generally keep under 80 chars per line.

sklearn/metrics/regression.py

whiletruelearn · 2018-10-10T08:04:18Z

@qinhanmin2014 @jnothman I have made the changes as per review comments. Can you please let me know if there are any other changes to make?

qinhanmin2014

Some formatting issues. It would be better if you check your PR carefully again :)

doc/modules/model_evaluation.rst

sklearn/metrics/regression.py

qinhanmin2014

LGTM, thanks @whiletruelearn

doc/modules/model_evaluation.rst

qinhanmin2014 · 2018-10-11T01:46:55Z

doc/whats_new/v0.21.rst

+:mod:`sklearn.metrics`
+......................
+
+- |Feature| A new regression metric: :class:`metrics.max_error`: a


this should be a sentence and you need to mention the scorer
see e.g., Added the metrics.balanced_accuracy_score metric and a corresponding 'balanced_accuracy' scorer for binary and multiclass classification. #8066 by @xyguo and Aman Dalmia, and #10587 by Joel Nothman.

Updated. Also changed it from :class: to :func:

qinhanmin2014

LGTM, thanks @whiletruelearn

whiletruelearn added 3 commits October 1, 2018 16:04

Add max error metric

e40258f

Fix typo

6905ee4

Fix some imports

0e92ee3

eamanu reviewed Oct 1, 2018

View reviewed changes

whiletruelearn changed the title ~~Add max_error to the existing set of metrics for regression~~ [WIP] Add max_error to the existing set of metrics for regression Oct 1, 2018

whiletruelearn added 2 commits October 1, 2018 18:43

Adding some test for max error

06f14d8

Add scorer for max error

ea9d3f0

whiletruelearn closed this Oct 1, 2018

qinhanmin2014 reopened this Oct 3, 2018

whiletruelearn added 2 commits October 3, 2018 14:20

Add max_error_scorer to fix the failing test

7b8222f

correct typo in scorer name

237759a

whiletruelearn changed the title ~~[WIP] Add max_error to the existing set of metrics for regression~~ [MRG] Add max_error to the existing set of metrics for regression Oct 3, 2018

qinhanmin2014 reviewed Oct 3, 2018

View reviewed changes

eamanu reviewed Oct 3, 2018

View reviewed changes

Changes based on review comments

9f0f6de

Sample weight initialization needs to be tweaked to make the test pass

495d6f0

jnothman approved these changes Oct 6, 2018

View reviewed changes

qinhanmin2014 approved these changes Oct 6, 2018

View reviewed changes

eamanu approved these changes Oct 6, 2018

View reviewed changes

whiletruelearn added 2 commits October 7, 2018 11:54

update what's new

361675a

Update the PR number also

5b82f09

Update the user guide

e296cdd

update the documentation based on review comments

ff35504

jnothman approved these changes Oct 8, 2018

View reviewed changes

qinhanmin2014 reviewed Oct 8, 2018

View reviewed changes

sklearn/metrics/regression.py Show resolved Hide resolved

Add reference and style fix for doc to have 80 char limit

cb50f23

qinhanmin2014 reviewed Oct 10, 2018

View reviewed changes

doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved

doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved

sklearn/metrics/regression.py Outdated Show resolved Hide resolved

Update review comments

f156d59

qinhanmin2014 approved these changes Oct 11, 2018

View reviewed changes

doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved

qinhanmin2014 changed the title ~~[MRG] Add max_error to the existing set of metrics for regression~~ [MRG+2] Add max_error to the existing set of metrics for regression Oct 11, 2018

qinhanmin2014 reviewed Oct 11, 2018

View reviewed changes

whiletruelearn added 3 commits October 11, 2018 11:00

Doc updates

3e0dfcf

Better structuring of sentence

a088a76

Missed to add the word metric

90d69db

qinhanmin2014 approved these changes Oct 11, 2018

View reviewed changes

qinhanmin2014 merged commit 831c760 into scikit-learn:master Oct 11, 2018

[MRG+2] Add max_error to the existing set of metrics for regression #12232

[MRG+2] Add max_error to the existing set of metrics for regression #12232

Conversation

whiletruelearn commented Oct 1, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

eamanu left a comment

Choose a reason for hiding this comment

eamanu commented Oct 1, 2018

qinhanmin2014 commented Oct 1, 2018

whiletruelearn commented Oct 1, 2018

qinhanmin2014 commented Oct 1, 2018

whiletruelearn commented Oct 1, 2018 • edited Loading

eamanu commented Oct 1, 2018

qinhanmin2014 commented Oct 2, 2018

qinhanmin2014 commented Oct 2, 2018

jnothman commented Oct 3, 2018 via email

whiletruelearn commented Oct 3, 2018

qinhanmin2014 commented Oct 3, 2018

whiletruelearn commented Oct 3, 2018 • edited Loading

qinhanmin2014 commented Oct 3, 2018

Choose a reason for hiding this comment

whiletruelearn Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whiletruelearn commented Oct 4, 2018

jnothman left a comment

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

qinhanmin2014 commented Oct 6, 2018

qinhanmin2014 commented Oct 6, 2018

eamanu commented Oct 6, 2018

whiletruelearn commented Oct 7, 2018

qinhanmin2014 commented Oct 7, 2018

whiletruelearn commented Oct 7, 2018

qinhanmin2014 commented Oct 7, 2018

whiletruelearn commented Oct 8, 2018

whiletruelearn commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whiletruelearn commented Oct 10, 2018

qinhanmin2014 left a comment

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

whiletruelearn commented Oct 1, 2018 •

edited

Loading

whiletruelearn commented Oct 3, 2018 •

edited

Loading

whiletruelearn Oct 4, 2018 •

edited

Loading