-
Notifications
You must be signed in to change notification settings - Fork 124
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
Support minimization and maximization of metrics #147
Conversation
@rgemulla the metric.py file was not added, so I can't test this yet |
Added. |
def __init__(self, metric_max: Union[Job, Config, bool]): | ||
"Params: metric_max=True means higher is better." | ||
if isinstance(metric_max, Job): | ||
metric_max = metric_max.config |
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.
Isn't the .get("valid.metric_max") missing here?
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.
The line right below does this.
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.
All looks good and also works well. Tested it with a couple of ax search runs, also tested early stopping explicitly. Just line 12 in metric.py seems off, the rest is fine!
Right! I automatically read those lines as mutually exclusive...
…On Thu, 17 Sep 2020, 23:13 Rainer Gemulla, ***@***.***> wrote:
Merged #147 <#147> into master.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#147 (comment)>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABEWXZDACUWNVQUMY4TSBCTSGJ3WDANCNFSM4RNS3OCQ>
.
|
via a new config key
valid.metric_max
@daniel: This is largely untested & I may also have missed some places where metrics are compared.