Skip to content
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 DiscreteUniformDistribution in suggest_float. #1081

Merged
merged 14 commits into from
Apr 17, 2020
Merged

Support DiscreteUniformDistribution in suggest_float. #1081

merged 14 commits into from
Apr 17, 2020

Conversation

himkt
Copy link
Member

@himkt himkt commented Apr 2, 2020

Description

This PR introduces step parameter to Trial.suggest_float.

Close #1069.

@codecov-io
Copy link

codecov-io commented Apr 4, 2020

Codecov Report

Merging #1081 into master will decrease coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
- Coverage   90.66%   90.37%   -0.30%     
==========================================
  Files         126      126              
  Lines       10954    11070     +116     
==========================================
+ Hits         9931    10004      +73     
- Misses       1023     1066      +43     
Impacted Files Coverage Δ
optuna/integration/chainermn.py 77.40% <100.00%> (ø)
optuna/trial.py 88.02% <100.00%> (+1.51%) ⬆️
tests/test_trial.py 98.19% <100.00%> (-1.13%) ⬇️
optuna/structs.py 51.28% <0.00%> (-30.84%) ⬇️
optuna/pruners/percentile.py 94.11% <0.00%> (-1.41%) ⬇️
optuna/storages/base.py 56.32% <0.00%> (-0.66%) ⬇️
tests/integration_tests/test_skopt.py 97.33% <0.00%> (ø)
optuna/storages/rdb/storage.py 96.01% <0.00%> (+<0.01%) ⬆️
optuna/storages/in_memory.py 97.86% <0.00%> (+0.01%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ece1b...bf3b9e5. Read the comment docs.

@himkt himkt marked this pull request as ready for review April 6, 2020 15:04
@himkt
Copy link
Member Author

himkt commented Apr 6, 2020

I made this PR ready for review since I want to discuss this PR with members.

In #1069, it seems that raising NotImplementedError is expected only when log=True and step!=1.
However, step would be negative when 0<step<1 and be undefined when step<=0.
When log=True and step<1, what behavior is expected?

When log=True and 1<step, I implemented suggest_float to call suggest_discrite_uniform with low=low, high=high and q=math.log(step). I'm not sure whether this implementation is correct.

Could anyone take a look at this PR, please?

@toshihikoyanase
Copy link
Member

In #1069, it seems that raising NotImplementedError is expected only when log=True and step!=1.

I'm really sorry but I made a mistake in #1069. The condition should be log=True and step is not None.
I think we may need to add a new distribution like LogDiscreteFloatDistribution and its corresponding calculation to support the condition. This PR can focus on the API design if we separate the PRs.

@himkt
Copy link
Member Author

himkt commented Apr 7, 2020

@toshihikoyanase Thank you for clarification, I revised my PR to raise NotImplementedError when
log=True and step is not None in e7cd5e8.

Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of API, the changes basically LGTM. I left some minor comments.

Just leaving a reference to another PR as they may conflict #1087.

optuna/trial.py Outdated Show resolved Hide resolved
optuna/trial.py Outdated Show resolved Hide resolved
@himkt
Copy link
Member Author

himkt commented Apr 8, 2020

@hvy I'm sorry and thank you for catching my violating conventions.
(In the thread of #1087, I couldn't see the reference to this PR...:thinking:)

@himkt
Copy link
Member Author

himkt commented Apr 8, 2020

Now I can see #1087 (reference).

@hvy
Copy link
Member

hvy commented Apr 8, 2020

Regarding how step may behave when log=True:

  1. SMAC3 (ConfigSpace) treats the "step" as given in the log space when log=True. https://github.com/automl/ConfigSpace/blob/master/ConfigSpace/hyperparameters.pyx#L383 This seems a bit unintuitive to me at first glance as the bounds are not, but I might be missing something.
  2. Hyperopt is clear by its API design, similar to SMAC3, "step" is given in the log space. https://github.com/hyperopt/hyperopt/blob/d87309463a1496cb66d83827afe204d0700e4aef/docs/templates/getting-started/search_spaces.md#parameter-expressions

So, it seems like treating the "step" in the log space is consistent with other libraries, but again, it's likely open for discussion.

@hvy hvy added the feature Change that does not break compatibility, but affects the public interfaces. label Apr 8, 2020
Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR and the quick fixed, LGTM!

@hvy hvy self-assigned this Apr 8, 2020
Copy link
Member

@toshihikoyanase toshihikoyanase left a 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 PR. I have two small comments.
FYI: the rendered document can be found https://35185-122299416-gh.circle-artifacts.com/0/docs/build/html/reference/trial.html#optuna.trial.Trial.suggest_float.

optuna/trial.py Outdated Show resolved Hide resolved
tests/test_trial.py Outdated Show resolved Hide resolved
Copy link
Member

@toshihikoyanase toshihikoyanase left a 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. I have two suggestions.

optuna/trial.py Outdated Show resolved Hide resolved
optuna/trial.py Outdated Show resolved Hide resolved
himkt and others added 2 commits April 13, 2020 21:10
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
@himkt
Copy link
Member Author

himkt commented Apr 17, 2020

@toshihikoyanase Sorry for not replying, I revised my PR. PTAL.

Copy link
Member

@toshihikoyanase toshihikoyanase left a 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 your contribution!

@toshihikoyanase toshihikoyanase merged commit 8910d6b into optuna:master Apr 17, 2020
@toshihikoyanase toshihikoyanase added this to the v1.4.0 milestone Apr 17, 2020
@himkt himkt deleted the suggest-float branch April 17, 2020 01:09
@himkt
Copy link
Member Author

himkt commented Apr 20, 2020

rel. #510

@sile sile mentioned this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change that does not break compatibility, but affects the public interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support discrete parameters by Trial.suggest_float().
4 participants