Skip to content
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.

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

Added mean_absolute_percentage_error in metrics fixes #10708 #15007

Merged
merged 120 commits into from Jul 4, 2020

Conversation

ashutosh1919
Copy link
Contributor

@ashutosh1919 ashutosh1919 commented Sep 18, 2019

I have faced problem when working with any quantitative modelling algorithm. Scikit-learn doesn't have any function such mean_absolute_percentage_error. Everytime, when I want to use this function, then I have to implement it myself. But, if it is included in sklearn itself then it will be great. I have written code for this function and I have also added tests for it. @agramfort , Please Review my PR.
mape
This PR Fixes #10708 . This PR is continuation of the work done by @mohamed-ali in #10711 PR.

@agramfort
Copy link
Member

agramfort commented Sep 18, 2019 via email

@ashutosh1919
Copy link
Contributor Author

It's check. If I don't put 1 in denomenator, then it will return inf when
y_true is 0 and then the value
returned by mean_absolute_percentage_error will be nan. To avoid this, I
am using 1 which won't
abruptly deviate the error value and also will be consistent. @agramfort
I would not do this. To me it's a feature not a bug that you can NaN / Inf if you have any y that is 0. If this happens in practice I presume it's bug in the dataset.

I was not using 1 previously but when I wrote test script for the function, one of the test case was given like below :

y_true = np.array([[1, 0, 0, 1], [0, 1, 1, 1], [1, 1, 0, 1]])
y_pred = np.array([[0, 0, 0, 1], [1, 0, 1, 1], [0, 0, 0, 1]])

Above input is completely valid input and looks like the output of the classifier. If I don't put 1 in denomenator, it will fail and gives me nan. But if I put 1 then it gives me correct answer 8.99 which I needed at last.

@jnothman
Copy link
Member

See an alternative contribution of this feature in #10711. I've not yet reviewed where it got stuck

@ashutosh1919
Copy link
Contributor Author

See an alternative contribution of this feature in #10711. I've not yet reviewed where it got stuck

I have seen the pending PR, should I raise error when any y_true is 0? If that is the case then I will write seperate test function for evaluating mean_absolute_percentage_error because the tests written for other error functions won't work for this one. Should I do that? or should I search for some better approach? Awaiting for reply @agramfort @jnothman

@ashutosh1919
Copy link
Contributor Author

See an alternative contribution of this feature in #10711. I've not yet reviewed where it got stuck

I have seen the pending PR, should I raise error when any y_true is 0? If that is the case then I will write seperate test function for evaluating mean_absolute_percentage_error because the tests written for other error functions won't work for this one. Should I do that? or should I search for some better approach? Awaiting for reply @agramfort @jnothman

@ashutosh1919
Copy link
Contributor Author

ashutosh1919 commented Sep 18, 2019

@jnothman @agramfort , Please look at this one. This is implementation of mean_absolute_percentage_error loss in tensorflow. They have clipped the smallest value to EPSILON. I think this is more robust approach than giving error. This will also work with every test case. If we raise on finding zero, most of the standard binary classification tasks will give error. But if we clip denominator to EPSILON, then it will work in every case.

@ashutosh1919 ashutosh1919 changed the title Added mean_absolute_percentage_error in metrics Added mean_absolute_percentage_error in metrics fixes #10708 Sep 18, 2019
@ashutosh1919
Copy link
Contributor Author

@amueller, @mohamed-ali, @jnothman, @agramfort
I have implemented MAPE using following formula and considering the comments which you have specified in Issue #10708 and PR #10711.
mape
Where epsilon is the smallest number greater than zero.
Implementation is based on np.finfo(float).eps which has value of 2.220446049250313e-16.
Also I have found implementation of MAPE in tensorflow losses here as shown below :

def mean_absolute_percentage_error(y_true, y_pred):  # pylint: disable=missing-docstring
  y_pred = ops.convert_to_tensor(y_pred)
  y_true = math_ops.cast(y_true, y_pred.dtype)
  diff = math_ops.abs(
      (y_true - y_pred) / K.clip(math_ops.abs(y_true), K.epsilon(), None))
  return 100. * K.mean(diff, axis=-1)

Above code shows that they have clipped the denominator value to epsilon lower bound.
I have implemented the same thing using numpy.

Advantages of this logic :
(i) Code will not give unexpected inf or nan value when used anywhere instead it will give very high error so that the developer will get to know that somewhere in the denominator, zero is encountered. It won't crash the model at any time instead it will show high error.
(ii) This logic doesn't require changing the test functions for this particular metrics function. Instead, we can test this function using the existing test cases and no other test cases are needed.
(iii) It doesn't violate the definition of MAPE which is shown in the Wikipedia and it won't deviate the error in the significant decimal points.

Current Logic Passes all test cases and implementation is robust as per tensorflow.

@agramfort
Copy link
Member

@ashutosh1919 you need to update classes.rst and doc/modules/model_evaluation.rst and scorer.py

a good way to find this out is to do a git grep on the related function eg git grep median_absolute_error

@ashutosh1919
Copy link
Contributor Author

@ashutosh1919 you need to update classes.rst and doc/modules/model_evaluation.rst and scorer.py

a good way to find this out is to do a git grep on the related function eg git grep median_absolute_error

I have added the doc for MAPE as per your instructions. @agramfort, Please take a look at doc. It is passes all tests.

@mohamed-ali
Copy link
Contributor

mohamed-ali commented Sep 20, 2019

@agramfort, @amueller @jnothman

I don't know how contribution are orchestrated in sklearn, but the issue #10708 has been addressed with my PR #10711.

The PR was approved by @jnothman, then remained unmerged for more than one year, and I have been recently asked by @amueller to synchronize it with master, but now I see this PR being created to address the exact same issue?

@agramfort @ashutosh1919 Could you please close the PR I created, or at least let me know there beforehand so I don't waste more time.

Thanks.

@ashutosh1919
Copy link
Contributor Author

@agramfort, @amueller @jnothman

I don't know how contribution are orchestrated in sklearn, but the issue #10708 has been addressed with my PR #10711.

The PR was approved by @jnothman, then remained unmerged for more than one year, and I have been recently asked by @amueller to synchronize it with master, but now I see this PR being created to address the exact same issue?

@agramfort @ashutosh1919 Could you please close the PR I created, or at least let me know there beforehand so I don't waste more time.

Thanks.

Sorry @mohamed-ali, At the time when I raised my PR, I didn't know that issue for the same is raised before and PR is already there on it. Thanks to @jnothman that he informed about your PR and also about the issue. By seeing the last update on your PR, I thought that there has not been any update since then. I am extremely sorry for that. But I have mentioned you in here so that you stay aware about the same issue. Hands over to @agramfort, Please close one of the PR (Mine or his) on same issue.

@ashutosh1919
Copy link
Contributor Author

Added mean_absolute_relative_error by PR #16689 .

@agramfort
Copy link
Member

agramfort commented Mar 17, 2020 via email

@ogrisel
Copy link
Member

ogrisel commented Mar 17, 2020

@agramfort to clarify your point: you want to only implement the mean_absolute_percentage_error score function (and the scorer) but without the multiplication by 100 (and therefore not implement mean_absolute_relative_error).

We would need to make it extra explicit in the documentation (user guide and docstring) that our implementation does not make the multiplication by 100.

I think @rth also agrees with this.

My original position expressed in #15007 (comment) was a bit different, but if others (in particular @jnothman @lesteve and @amueller who participated in the review of #10711) also agree with this choice, then I am fine with it.

@jnothman
Copy link
Member

jnothman commented Mar 17, 2020 via email

@agramfort
Copy link
Member

agramfort commented Mar 18, 2020 via email

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Otherwise this is looking good

doc/whats_new/_contributors.rst Outdated Show resolved Hide resolved
sklearn/metrics/_regression.py Outdated Show resolved Hide resolved
sklearn/metrics/_regression.py Show resolved Hide resolved
sklearn/metrics/_regression.py Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Mar 19, 2020

We could invent mean_absolute_proprotional_error to have the same acronym??!

It looks to much like a hack and feels un-natural to me. I think would rather either:

  • stick to mean_absolute_relative_error and just mention the MAPE acronym and its expansion in the docstring and user guide instead.
  • or just use the name mean_absolute_percentage_error without the x100 operation as Wikipedia does it.

@agramfort
Copy link
Member

agramfort commented Mar 19, 2020 via email

@ashutosh1919
Copy link
Contributor Author

@jnothman , I have updated the code as per your last review.
@agramfort, @ogrisel, @jnothman, @amueller, @rth - Note that I have removed x100 from the MAPE code and now it will give error for all valid inputs in the range [0,1]. Please review the latest code.

Now, the code in this PR and #16689 for MARE are same other than the difference of names.
If we are going ahead with this one. Then please close #16689 and the corresponding issue.

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

One last think: doc/modules/model_evaluation.rst lists scorers. neg_mean_absolute_percentage_error is absent there.

@rth rth added this to the 0.24 milestone Jul 3, 2020
@rth rth self-requested a review July 3, 2020 19:32
@rth rth merged commit 0ea3244 into scikit-learn:master Jul 4, 2020
@rth
Copy link
Member

rth commented Jul 4, 2020

I have addressed the last comment. Merging with +3. Thank you!

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
 (scikit-learn#15007)

Co-authored-by: mohamed-ali <m.ali.jamaoui@gmail.com>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Joel Nothman <joel.nothman@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@pm.me>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
 (scikit-learn#15007)

Co-authored-by: mohamed-ali <m.ali.jamaoui@gmail.com>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Joel Nothman <joel.nothman@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@pm.me>
@ShivKJ
Copy link

ShivKJ commented May 19, 2022

Wiki mentions multiplication by 100. Not Multiplying by 100 (current impl) seems so unnatural.

We could invent mean_absolute_proprotional_error to have the same acronym??!

It looks to much like a hack and feels un-natural to me. I think would rather either:

  • stick to mean_absolute_relative_error and just mention the MAPE acronym and its expansion in the docstring and user guide instead.
  • or just use the name mean_absolute_percentage_error without the x100 operation as Wikipedia does it.

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.

Add MAPE as evaluation metric