-
Notifications
You must be signed in to change notification settings - Fork 65
[ENH] sign handling in experiments and optimization #142
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
| # if "deterministic", two calls of score must result in the same value | ||
| # | ||
| "property:higher_or_lower_is_better": "lower", | ||
| # valid values: "higher", "lower", "mixed" |
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.
What is mixed? It is not handled by the cost method.
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.
in theory there could be an experiment that is neither "lower is better" nor "higher is better" - an example would be a metric such as "empirical coverage of 90% nominal prediction intervals", where the best value is 90% (not 0% or 100%)
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.
Please update readme.md (and docstrings?). They still speak of "higher is better" or maximization in several places.
Can you point towards these? Stupid question, also: what is the current consensus in gfo and the adapter methods? Maximize, or minimize? |
Just search for higher/maximize in the readme. I take care of the api docs.
gfo maximizes the objective function in the current api. But I aim for support for minimize/maximize as well. |
|
ouch - I will then flip the sign as an action, is that what I should do? So it is consistently maximize? What is your preferred action?
? That way it would be consistent with the |
|
@fkiraly Let's flip it to maximize and leave the docstrings. I'll go through the code again, but we can probably merge after that. |
|
ok, that is also more consistent with |
|
I think this is ready now - I have made the following changes:
I think this is an elegant way to minimize the extension boilerplate for the optimizers, which now are assumed to always maximize. |
18f8a0f
into
hyperactive-project:master
Fixes #141 by the following approach:
property:higher_or_lower_is_betterto experiments to signifiy whether this is minmization or maximizationscoremethod is always "higher is better"evaluate/_evaluate, which has the same orientation as the new tagproperty:higher_or_lower_is_better. The_scoremethod no longer exists.The
SklearnCvExperimentalso gets internal functionality to detect the sign from the metric passed. Since metrics insklearnare not tagged properly, there is some clunky detection logic to infer this non-existent tag.Further changes:
property:higher_or_lower_is_better_scoremethods are changed to_evaluateDecision to move to maximization, see discussion in #141.