-
-
Notifications
You must be signed in to change notification settings - Fork 970
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 constrained optimization in best_trial
#5426
Support constrained optimization in best_trial
#5426
Conversation
@HideakiImamura @nabenabe0928 Could you review this PR? |
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.
Thank you for the PR, I left some comments!
# violation value in the best-valued trial. | ||
constraints = best_trial.system_attrs.get(_CONSTRAINTS_KEY) | ||
if constraints is not None and any([x > 0.0 for x in constraints]): | ||
complete_trials = self.get_trials(deepcopy=False, states=[TrialState.COMPLETE]) |
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.
Is it possible to use cache here or would it be buggy because some trials might finish between the last query and this query of self.get_trials
?
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 my understanding, the cache in _get_trials
is taken at the beginning of the trial, so it is not appropriate to use that cache in best_trial
. It can be a strange value since best_trial
is expected to be called after the trial.
optuna/study/study.py
Outdated
feasible_trials = [] | ||
for trial in trials: | ||
constraints = trial.system_attrs.get(_CONSTRAINTS_KEY) | ||
if constraints is None or all([x <= 0.0 for x in constraints]): |
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.
TPE and NSGA-II consider constraints is None
worse than trials with constraints is not None
, implying that constraints is None
is infeasible.
- https://github.com/optuna/optuna/blob/master/optuna/samplers/_tpe/sampler.py#L758-L768
- https://github.com/optuna/optuna/blob/master/optuna/samplers/nsgaii/_constraints_evaluation.py#L47-L53
if constraints is None or all([x <= 0.0 for x in constraints]): | |
if constraints is not None and all(x <= 0.0 for x in constraints): |
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.
Thank you for your suggestion. I've updated it and _multi_objective.py
's one. There is some duplicate code, but I will do the refactoring in another PR.
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
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.
Almost, LGTM. I have one comment. Could you take a look?
optuna/study/study.py
Outdated
@@ -58,6 +59,15 @@ class _ThreadLocalStudyAttribute(threading.local): | |||
cached_all_trials: list["FrozenTrial"] | None = None | |||
|
|||
|
|||
def _get_feasible_trials(trials: Sequence[FrozenTrial]) -> list[FrozenTrial]: |
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.
It seems that this is identical to that of study/_multi_objective.py
. Can we remove either one?
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.
LGTM after addressing the mamu's comment!
This pull request has not seen any recent activity. |
I added |
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.
Sorry for the late. LGTM.
Motivation
Resolve #5184.
This change breaks backward compatibility but results in intuitive behavior for constrained optimization.
Description of the changes
best_trial
: We need to check all feasible trials to support constrained optimization inbest_trial
while keeping the current storage schema. However, there is a concern about speed degradation if all trials are checked, so thebest_trial
is first determined without regard to constraints, and all trials are checked only if the best-valued trial violates constraints.best_trials
: Sincebest_trials
needs to calculate the Pareto front, the impact of filtering on speed should be small.