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

Implement RMSE (root-mean-square error) metric and scorer #13467

Merged
merged 13 commits into from Aug 7, 2019

Conversation

urvang96
Copy link
Contributor

@urvang96 urvang96 commented Mar 18, 2019

Reference Issues/PRs
Fixes #12895
Implement RMSE (root-mean-square error) metric and scorer

What does this implement/fix? Explain your changes.
Added a boolean parameter in MSE implementation to return RMSE value, if set to true. Also added an RMSE scorer.

Any other comments?
One test.

@urvang96
Copy link
Contributor Author

@urvang96 urvang96 commented Mar 18, 2019

As per my understanding, I have added the scorer. Let me know if I missed something.
Thank you in advance.

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Mar 19, 2019

there're still some pep8 errors, please refer to https://circleci.com/gh/scikit-learn/scikit-learn/52068

@urvang96 urvang96 changed the title [WIP]Implement RMSE (root-mean-square error) metric and scorer Implement RMSE (root-mean-square error) metric and scorer Mar 22, 2019
sklearn/metrics/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/regression.py Outdated Show resolved Hide resolved
sklearn/metrics/scorer.py Outdated Show resolved Hide resolved
@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Mar 30, 2019

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Mar 30, 2019

I think we need more tests, e.g., test_regression_multioutput_array and test_regression_metrics_at_limits.

@xinyuliu12
Copy link
Contributor

@xinyuliu12 xinyuliu12 commented Apr 20, 2019

May I try continuing work on this?

@urvang96
Copy link
Contributor Author

@urvang96 urvang96 commented Apr 20, 2019

@xinyuliu12 Actually I am waiting for a review on this. Also, I am not sure about the doc failures in the commit so if you want you can help with that.

@qinhanmin2014 Could you please review and let me know if any changes required.

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Apr 23, 2019

@urvang96 Merge with master should fix the doc tests

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

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 :pr: and credit yourself (and other contributors if applicable) with :user:.

@urvang96
Copy link
Contributor Author

@urvang96 urvang96 commented Aug 5, 2019

@qinhanmin2014 Added the changes in whats_new. Thank you.

sklearn/metrics/regression.py Outdated Show resolved Hide resolved
@@ -253,7 +260,11 @@ def mean_squared_error(y_true, y_pred,
# pass None as weights to np.average: uniform mean
multioutput = None

return np.average(output_errors, weights=multioutput)
mse = np.average(output_errors, weights=multioutput)
if not squared:
Copy link
Member

@thomasjpfan thomasjpfan Aug 7, 2019

Choose a reason for hiding this comment

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

Nit:

return mse if squared else np.sqrt(mse)

Copy link
Contributor Author

@urvang96 urvang96 Aug 7, 2019

Choose a reason for hiding this comment

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

rth
rth approved these changes Aug 7, 2019
Copy link
Member

@rth rth left a comment

LGTM as well, thanks @urvang96 !

@rth rth merged commit 6425a8f into scikit-learn:master Aug 7, 2019
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants