-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
Deprecated negative valued scorers #6028
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
Conversation
'mean_absolute_error' :func:`metrics.mean_absolute_error` | ||
'mean_squared_error' :func:`metrics.mean_squared_error` | ||
'median_absolute_error' :func:`metrics.median_absolute_error` | ||
'neg_mean_absolute_error' :func:`metrics.mean_absolute_error` Negative MAE returned so greater=better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this explanation is the best possible but I don't have a better one ;)
\
So the problem here is that The way I propose to solve this, is to deprecate the message = "Function returns negative %s so that greater=better. This function is deprecated in version 0.18 and will be removed in version 0.20. Use neg_mean_squared_error instead"
mean_squared_error_scorer.__call__ = deprecated(message % "MSE")(mean_squared_error_scorer.__call__) above that you have to define a |
(I also defined the message as an extra variable so it is in the source code only once.) |
hm... I just realized that this object can then no longer be pickled. So this solution doesn't actually work. Maybe @ogrisel @agramfort @GaelVaroquaux or @jnothman have ideas how to solve this? My best idea at the moment would be to add an additional "deprecation" parameter that takes a message, and the scorer classes store this in an attribute, and when they are called, they check if the attribute is set, and if so, raise a deprecation warning. something like adding in if self.deprecation is not None:
raise DeprecationWarning(self.deprecation) and similarly in the other scorers. |
The scorers are not part of the public API, so should not be deprecated like that (apart from the verbosity of the code where a loop should be used instead). The deprecation message should be triggered in So |
IMO, |
Hmm... When I previously needed deprecation of scorers (but not in the merged version of the code), I also introduced a |
It's a bit unclear currently whether Maybe that was not a good idea and it should be private and the deprecation should be in |
Sorry, I've lost that code. I think I may have defined a SCORER_DEPRECATION On 16 December 2015 at 02:49, Andreas Mueller notifications@github.com
|
@lvrzhn could you kindly update us if you are still interested/available to work on this PR? |
Yes, I am still interested in collaborating on this! |
First could you rebase please? |
@lvrzhn This is an important issue, would you mind if I cherry picked your commit (you will be the author) and continued on your work in a separate PR? |
Hi raghavr, sorry for the delay. I wouldn't mind, please go ahead. Thanks a lot. |
replaced by #7261. |
deprecated X = mean_squared_error, mean_absolute_error, median_absolute_error, log_loss, when called, print "Function returns negative of X so that greater=better. This function is deprecated in version
0.18 and will be removed in a future version 0.20. Use neg_* instead"
replaced X_scorer by neg_X_scorer throughout scikit-learn source code as well as tests. Documented in doc/modules/model_evaluation.rst
Still need to do:
3) add a new test that checks if you use deprecated one you get deprecation warning and same behavior