-
-
Notifications
You must be signed in to change notification settings - Fork 962
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
Update cli tutorial #3902
Update cli tutorial #3902
Conversation
@@ -847,7 +847,7 @@ def get_parser(self, prog_name: str) -> ArgumentParser: | |||
"--skip-if-finished", | |||
default=False, | |||
action="store_true", | |||
help="If specified, tell is skipped without any error when the trial is already" | |||
help="If specified, tell is skipped without any error when the trial is already " |
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.
Currently, this part is printed as alreadyfinished
.
# Even so, we can invoke the optimization as follows. | ||
# (Don't care about ``--storage sqlite:///example.db`` for now, which is described in :ref:`rdb`.) |
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'd like to suggest removing (
and )
.
…d in CLI tutorial
8ca67ad
to
4fda221
Compare
Codecov Report
@@ Coverage Diff @@
## master #3902 +/- ##
==========================================
- Coverage 90.58% 90.57% -0.02%
==========================================
Files 164 164
Lines 12601 12601
==========================================
- Hits 11415 11413 -2
- Misses 1186 1188 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@keisuke-umezawa Could you review this PR, please? I think we can merge this PR with a single approval since it updates the tutorial only. |
# ... | ||
# [I 2018-05-09 10:40:26,204] Finished a trial resulted in value: 14.704254135013741. Current best value is 2.280758099793617e-06 with parameters: {'x': 1.9984897821018828}. | ||
# $ optuna ask --storage sqlite:///example.db --study-name $STUDY_NAME --sampler TPESampler \ | ||
# --search-space '{"x": {"name": "FloatDistribution", "attributes": {"step": null, "low": -10.0, "high": 10.0, "log": false}}}' |
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.
Do we actually need to specify step
and log
here? If this commend can run without these arguments, we can remove them. Some people might get stuck by them.
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.
No, we don't. However, I believe that we use https://optuna.readthedocs.io/en/stable/reference/generated/optuna.distributions.distribution_to_json.html to obtain the json formatted distribution, which includes step
and log
as seen above.
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 do you think about
- removing
log
andstep
key values from the example above for simplicity and readability, and - Adding a sentence to mention https://optuna.readthedocs.io/en/stable/reference/generated/optuna.distributions.distribution_to_json.html to generate json formatted distribution?
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 understand. In that case, to generate the json formatted distribution, we need to explain how we can create it. I agree on your idea.
Adding a sentence to mention https://optuna.readthedocs.io/en/stable/reference/generated/optuna.distributions.distribution_to_json.html to generate json formatted distribution?
You can explain it by the above. But, users need to run some code to generate it. The other way is that you can explain it by some sentences like:
You can choose distributions from `FloatDistribution` and `IntDistribution`, and
specify parameters by the key values of json with `low`, `high`, `log` and `step`.
If you want to use log scale, you can set `"log": true`, otherwise `"log": false`.
If you want to specify step e.g. 2, you can set `"step": 2`, otherwise `"step": null`.
The above sentences is just a quick job, so we need to refine it.
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. I think the explanation of each argument of the distribution looks unnecessary for this tutorial, so I've added only links to https://optuna.readthedocs.io/en/stable/reference/generated/optuna.distributions.distribution_to_json.html t and distributions. For the consistency with the output of distribution_to_json
, I've not removed log
and step
from the example code.
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 updating it with a more concrete example. I left a comment. Could you check it?
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 updates.
Motivation
Resolve #3032.
Description of the changes
As described in #3032, remove deprecated command examples and use ask and tell commands. However, since the
ask
andtell
commands in CLI module doesn't look suitable for the current example codefoo.py
, I've created another scenario to use these commands.