-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Display conditional parameters properly in TrialTable #740
Display conditional parameters properly in TrialTable #740
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #740 +/- ##
==========================================
+ Coverage 68.22% 69.71% +1.48%
==========================================
Files 35 35
Lines 2329 2364 +35
==========================================
+ Hits 1589 1648 +59
+ Misses 740 716 -24 ☔ View full report in Codecov by Sentry. |
@keisuke-umezawa Could you review this PR? |
Let me assign @porink0424 as an additional reviewer 🙏 |
Sure. |
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!
@keisuke-umezawa Please merge this PR after you approved 🙏 |
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 being late. I left one comment. Could you check it?
trials[i].params.find((p) => p.name === s.name)?.param_external_value || | ||
null, | ||
sortable: sortable, | ||
filterChoices: isDynamicSpace ? filterChoicesWithNull : filterChoices, |
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.
isDynamicSpace
is calculated for each study, but filterChoices
is set for each column. This isDynamicSpace
should be implemented as the value whether the column is dynamic space.
import optuna
def objective(trial):
category = trial.suggest_categorical("category", ["foo", "bar"])
if category == "foo":
return (trial.suggest_float("x1", 0, 10) - 2) ** 2
else:
return -((trial.suggest_float("x2", -10, 0) + 5) ** 2)
study = optuna.create_study(storage="sqlite:///test.db")
study.optimize(objective, n_trials=100)
In the above code, this study's isDynamicSpace
is always true because some of columns are dynamic spaces. As a result, filterChoices
will be filterChoicesWithNull
even though category
is not dynamic space.
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.
Or, something like this?
const filterChoices =
s.distribution.type === "CategoricalDistribution"
? s.distribution.choices.map((c) => c.value)
: undefined
values = trials[i].params.find((p) => p.name === s.name)?.param_external_value
if values.contain(null):
filterChoices.append(null)
275542a
to
c63f15b
Compare
c63f15b
to
c13654f
Compare
@keisuke-umezawa Thank you for the suggestion, I addressed your point! |
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 quick fix.
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 PR solves issue#734.
What does this implement/fix? Explain your changes.
By this PR, TrialTable can: