-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
negate the result and prefix the metric name for error/loss metrics #278
Conversation
@Innixma does this look reasonable? |
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.
Prefixing neg_
is probably more sensible (since scikit-learn does it) so I agree with that choice even though it takes more space. Other than that I tested it (with constant predictor) and it looks good.
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.
Apologies for late response, I was on PTO.
Looks good to me, had a comment for long-term improving code quality and extensibility.
def auc(self): | ||
"""Array Under (ROC) Curve, computed on probabilities, not on predictions""" |
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.
nit: area instead of array
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.
oups! will fix
return float(r2_score(self.truth, self.predictions)) | ||
|
||
|
||
def higher_is_better(metric): |
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.
This seems a bit hacky. Better to have either a dictionary mapping or metrics as classes (example in AutoGluon).
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 can't disagree with you: it IS a bit hacky.
Ideally, there should be a class for each metric. It's probably something I'll do at some point to support custom metrics or other customizations in a more satisfying way that what was done in #141.
If there's a demand for it, I'll do it.
#268
Only changing the
result
andmetric
columns when the metric represents an error:metric
name is prefixed withneg_
.result
is negated.Example: