Conversation
|
@fusawa-yugo @not522 @gen740 Could you review this PR? |
gen740
left a comment
There was a problem hiding this comment.
Thank you for your PR!
I left several comments.
| warnings.warn( | ||
| "`target` is specified, but `target_name` is the default value, 'Objective Value'." | ||
| "`target` is specified, but `target_name` is the default value, 'Objective Value'.", | ||
| stacklevel=2, |
There was a problem hiding this comment.
Currently, this wrapper function is used by following code locations:
optuna/visualization/_contour.py|261 col 5| _check_plot_args(study, target, target_name)
optuna/visualization/_edf.py|111 col 5| _check_plot_args(studies, target, target_name)
optuna/visualization/_optimization_history.py|53 col 5| _check_plot_args(study, target, target_name)
optuna/visualization/_parallel_coordinate.py|124 col 5| _check_plot_args(study, target, target_name)
optuna/visualization/_param_importances.py|39 col 5| _check_plot_args(study, target, target_name)
optuna/visualization/_rank.py|118 col 5| _check_plot_args(study, target, target_name)
optuna/visualization/_slice.py|96 col 5| _check_plot_args(study, target, target_name)
All of these, except for _param_importances.py, work correctly with stacklevel=4. But for _param_importances.py, it should be 5.
Therefore, I think _check_plot_args itself should accept a stacklevel parameter.
I used the following code to verify this behavior.
import optuna
from optuna.visualization import (
plot_contour,
plot_edf,
plot_optimization_history,
plot_parallel_coordinate,
plot_param_importances,
plot_rank,
plot_slice,
)
def optimize_function(trial):
x = trial.suggest_float("x", -10, 10)
return (x - 2) ** 2
study = optuna.create_study(
study_name="example_study",
)
study.optimize(optimize_function, n_trials=50)
fig = plot_contour(study, target=lambda t: t.value)
fig = plot_edf(study, target=lambda t: t.value)
fig = plot_optimization_history(study, target=lambda t: t.value)
fig = plot_parallel_coordinate(study, target=lambda t: t.value)
fig = plot_param_importances(study, target=lambda t: t.value)
fig = plot_rank(study, target=lambda t: t.value)
fig = plot_slice(study, target=lambda t: t.value)
fig.show()There was a problem hiding this comment.
I also confirmed stacklevel of each call.
There was a problem hiding this comment.
To handle different stack levels depending on how users call the function, how about passing stack_level as an argument to _check_plot_args, like this:
def _check_plot_args(
study: Study | Sequence[Study],
target: Callable[[FrozenTrial], float] | None,
target_name: str,
stacklevel: int,
) -> None:
studies: Sequence[Study]
if isinstance(study, Study):
studies = [study]
else:
studies = study
if target is None and any(study._is_multi_objective() for study in studies):
raise ValueError(
"If the `study` is being used for multi-objective optimization, "
"please specify the `target`."
)
if target is not None and target_name == "Objective Value":
warnings.warn(
"`target` is specified, but `target_name` is the default value, 'Objective Value'.",
stacklevel=stacklevel,| name="`axis_order`", d_ver="3.0.0", r_ver="5.0.0" | ||
| ) | ||
| warnings.warn(msg, FutureWarning) | ||
| warnings.warn(msg, FutureWarning, stacklevel=2) |
There was a problem hiding this comment.
| warnings.warn(msg, FutureWarning, stacklevel=2) | |
| warnings.warn(msg, FutureWarning, stacklevel=3) |
plot_pareto_front -> _get_pareto_front_info -> warn
| name="`constraints_func`", d_ver="4.0.0", r_ver="6.0.0" | ||
| ) | ||
| warnings.warn(msg, FutureWarning) | ||
| warnings.warn(msg, FutureWarning, stacklevel=2) |
There was a problem hiding this comment.
| warnings.warn(msg, FutureWarning, stacklevel=2) | |
| warnings.warn(msg, FutureWarning, stacklevel=3) |
Ditto
fusawa-yugo
left a comment
There was a problem hiding this comment.
Thank you for submitting PR!
I left some comments.
I think the importance of where to place warnings varies depending on the feature, so it might be a good idea to split the PRs by feature.
Also, just to confirm, this PR aims to display warnings at the user’s call stack level, right?
|
@ktns |
|
This pull request has not seen any recent activity. |
|
This pull request was closed automatically because it had not seen any recent activity. If you want to discuss it, you can reopen it freely. |
Motivation
While testing some code, I encountered the following warning message:
In this case, the issue lies not with the called function itself but with the arguments provided by the caller. Therefore, displaying the caller information would be more helpful for users I think.
Description of the changes
I have added
stackleveltowarnings.warncalls in the above line and some other relevant locations.I would be grateful if you could review this change.