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
Apply experimental decorator. #1424
Apply experimental decorator. #1424
Conversation
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 just started the review of this PR, but let me leave a comment.
optuna/dashboard.py
Outdated
def _show_experimental_warning(): | ||
# type: () -> None | ||
|
||
logger = optuna.logging.get_logger(__name__) | ||
logger.warning("Optuna dashboard is still highly experimental. Please use with caution!") | ||
warnings.warn("Optuna dashboard is still highly experimental. Please use with caution!") |
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 message shown in the console is as follows:
optuna/_experimental.py:63: ExperimentalWarning:
_show_experimental_warning is experimental (supported from v0.1.0). The interface can change in the future.
optuna/dashboard.py:264: UserWarning:
Optuna dashboard is still highly experimental. Please use with caution!
- I think the target of experimental warning is the dashboard, not
_show_experimental_warning
- The warning was divided into two. I think multiple warnings for a single issue may be confusing. Do we add the
text
argument to@experimental
like@deprecated
?
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 your comments. I totally agree with you.
- I will specify the
name
argument for the experimental decorator. - I will remove the second warning because it is sufficient here to note the default message of the experimental decorator. BTW, it might be good to give the
text
argument to the experimental decorator.
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!
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 update. LGTM!
Motivation
Apply the experimental decorator to classes which are experimental without the experimental decorator.
Description of the changes
This PR applies the experimental decorator to the following classes:
optuna.dashboard
optuna.integration.sklearn.OptunaSearchCV