-
-
Notifications
You must be signed in to change notification settings - Fork 92
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 for timeline plot #678
Conversation
Codecov Report
@@ Coverage Diff @@
## main #678 +/- ##
=======================================
Coverage 62.88% 62.88%
=======================================
Files 35 35
Lines 2250 2250
=======================================
Hits 1415 1415
Misses 835 835 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
@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")
if (s == "Complete") { | ||
const feasibleTrials = bars.filter((t) => | ||
t.constraints.every((c) => c <= 0) | ||
) | ||
const infeasibleTrials = bars.filter((t) => | ||
t.constraints.some((c) => c > 0) | ||
) | ||
if (feasibleTrials.length > 0) { | ||
traces.push(makeTrace(feasibleTrials, "Complete", cm[s])) | ||
} | ||
if (infeasibleTrials.length > 0) { | ||
traces.push(makeTrace(infeasibleTrials, "Infeasible", "#cccccc")) | ||
} | ||
} else { | ||
traces.push(makeTrace(bars, s, cm[s])) | ||
} |
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 about early return?
if (s == "Complete") { | |
const feasibleTrials = bars.filter((t) => | |
t.constraints.every((c) => c <= 0) | |
) | |
const infeasibleTrials = bars.filter((t) => | |
t.constraints.some((c) => c > 0) | |
) | |
if (feasibleTrials.length > 0) { | |
traces.push(makeTrace(feasibleTrials, "Complete", cm[s])) | |
} | |
if (infeasibleTrials.length > 0) { | |
traces.push(makeTrace(infeasibleTrials, "Infeasible", "#cccccc")) | |
} | |
} else { | |
traces.push(makeTrace(bars, s, cm[s])) | |
} | |
if (s !== "Complete") { | |
traces.push(makeTrace(bars, s, cm[s])) | |
continue | |
} | |
const feasibleTrials = bars.filter((t) => | |
t.constraints.every((c) => c <= 0) | |
) | |
const infeasibleTrials = bars.filter((t) => | |
t.constraints.some((c) => c > 0) | |
) | |
if (feasibleTrials.length > 0) { | |
traces.push(makeTrace(feasibleTrials, "Complete", cm[s])) | |
} | |
if (infeasibleTrials.length > 0) { | |
traces.push(makeTrace(infeasibleTrials, "Infeasible", "#cccccc")) | |
} |
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 don't think early return is appropriate here since the codes inside the brackets are at the same level. For example, if we add another if statement for "Waiting", it should be like this.
if (s === "Complete") {
...
} else if (s === "Waiting") {
...
} else {
...
}
not like this.
if (s !== "Complete" && s !== "Waiting") {
...
continue
}
if (s === "Complete") {
...
continue
}
...
if (bars.length === 0) { | ||
continue | ||
} | ||
const makeTrace = (bars: Trial[], name: string, color: string) => { |
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 is a very minor comment, but what about replacing name
with state
?
const makeTrace = (bars: Trial[], name: string, color: string) => { | |
const makeTrace = (bars: Trial[], state: string, color: string) => { |
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 minor comment. PTAL.
Thank you for your reviews. I have updated this PR responding to your comments. PTAL. |
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 update. LGTM.
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 update! LGTM
Contributor License Agreement
This repository (
optuna-dashboard
) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
This feature corresponds to optuna/optuna#4877.
What does this implement/fix? Explain your changes.
This PR supports constrained optimization for timeline plot. Same as Optuna, it applies only to
COMPLETE
trials.