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
Add log-linear algorithm for 2d Pareto front. #2503
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2503 +/- ##
==========================================
+ Coverage 89.78% 89.83% +0.04%
==========================================
Files 135 135
Lines 11308 11338 +30
==========================================
+ Hits 10153 10185 +32
+ Misses 1155 1153 -2
Continue to review full report at Codecov.
|
Thank you for the great improvement. Tests are okay, and outputs also look solid. -- Casual benchmark taken with pytest-benchmark, code: https://github.com/crcrpar/optuna/tree/benchmark-new-2dparetofront This pull request
Current master branch
|
@HideakiImamura |
I have pushed another commit because the algorithm was missing an edge case. For example, if the directions are I tried to run tests twice, but it looks like they are failing due to an unrelated issue:
For what it's worth, tests pass locally on my machine. |
It seems like the latest mxnet caused that error, and I confirmed the tests on which this pull request has some effect pass: https://github.com/optuna/optuna/pull/2503/checks?check_run_id=2161074877#step:8:36. Sorry, but |
No worries. I'll just leave it as is unless you need a passing ✔️ to merge. |
PR #2508 fixed the MXNet problem. Please re-base with 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.
LGTM. Thank you for the PR!
Done. Tests pass now. Thank you for your review! |
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.
Thanks for the PR! I have a comment to improve the code readability. Could take a look?
@@ -8,7 +8,46 @@ | |||
from optuna.trial import TrialState | |||
|
|||
|
|||
def _get_pareto_front_trials(study: "optuna.study.BaseStudy") -> List[FrozenTrial]: | |||
def _get_pareto_front_trials_2d(study: "optuna.study.BaseStudy") -> 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.
How about sorting the trials themself, and using _dominates
function as done in _get_pareto_front_trials_nd
?
pareto_front = []
trials = [trial for trial in study.trials if trial.state == TrialState.COMPLETE]
trials.sort(key=lambda t: _normalize_value(t.values[1], study.directions[1]))
trials.sort(key=lambda t: _normalize_value(t.values[0], study.directions[0]))
last_nondominated_trial: Optional[FrozenTrial] = None
for i in range(len(trials)):
if i == 0:
pareto_front.append(trials[0])
last_nondominated_trial = trials[0]
continue
assert last_nondominated_trial is not None
if not _dominates(last_nondominated_trial, trials[i], study.directions):
pareto_front.append(trials[i])
last_nondominated_trial = trials[i]
This code does not preserve the order of all trials and pareto front trials. so it will be fixed.
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 like the idea of using _dominates
to simplify the code. I implemented your suggestion.
The only difference between my implementation and your suggestion is that I am still using mask
because the order of trials needs to be preserved when filtering (as you point out in your comment).
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.
To preserve the order of trials, we only need to sort the Pareto front trials with its trial.number
. Could you remove the mask
? (I have noticed this simple fix after posting the above messages. Sorry.)
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.
Nice catch. Done.
def _get_pareto_front_trials(study: "optuna.study.BaseStudy") -> List[FrozenTrial]: | ||
if len(study.directions) == 2: | ||
return _get_pareto_front_trials_2d(study) # Log-linear in number of trials. | ||
return _get_pareto_front_trials_nd(study) # Quadratic in number of 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.
The non-2d scenario seems not to be covered by any test case in test_study.py
. We may want to add new ones, since it's risky to leave the general logic not tested.
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.
Added a 3d test case.
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!
Motivation
study.best_trials
can be made faster when there are only two objectives.Description of the changes
Uses a log-linear algorithm algorithm to construct the Pareto frontier in the case of two objectives that works as follows: