Skip to content

Fix the method to calculate n_brackets in HyperbandPruner.#1188

Merged
crcrpar merged 8 commits intooptuna:masterfrom
HideakiImamura:refactor/hyperband/fix_calc_n_brackets
May 1, 2020
Merged

Fix the method to calculate n_brackets in HyperbandPruner.#1188
crcrpar merged 8 commits intooptuna:masterfrom
HideakiImamura:refactor/hyperband/fix_calc_n_brackets

Conversation

@HideakiImamura
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura commented Apr 29, 2020 •
edited
Loading

Motivation

In the current implementation, we do not consider min_resource to calculate n_brackets in HyperbandPruner. This leads to an unrealistic number of brackets when min_resource is large. This PR aims to fix that issue.

Description of the changes

  • Fix the method to calculate n_brackets as follows.
    n_brackets = floor(log_{reduction_factor}(max_resource / min_resource)) + 1

@nzw0301
Copy link
Copy Markdown
Member

nzw0301 commented Apr 29, 2020 •
edited
Loading

Related to #1182, math.floor(math.log(max_resource / min_resource, reduction_factor)) + 1 might be better.

hvy
hvy approved these changes Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Great, LGTM!

If you want, you could change to math.log as commneted by @nzw0301 and we could close #1182.

@hvy hvy added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Apr 30, 2020
@hvy hvy added this to the v1.4.0 milestone Apr 30, 2020
@hvy hvy added the optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions. label Apr 30, 2020
@HideakiImamura
Copy link
Copy Markdown
Member Author

HideakiImamura commented Apr 30, 2020

@hvy Thank you for your review. I changed to use math.log at the latest version.

hvy
hvy approved these changes Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
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.

LGTM!

We might want to update the docs of the max_resource argument description. We can do that in a different PR, though.

@hvy hvy self-assigned this Apr 30, 2020
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 30, 2020 •
edited
Loading

Codecov Report

Merging #1188 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1188   +/-   ##
=======================================
  Coverage   91.06%   91.06%           
=======================================
  Files         142      142           
  Lines       12282    12287    +5     
=======================================
+ Hits        11184    11189    +5     
  Misses       1098     1098
Impacted Files Coverage Δ
optuna/pruners/hyperband.py 92.75% <ø> (ø)
optuna/integration/lightgbm_tuner/optimize.py 83.18% <100.00%> (+0.19%) ⬆️
optuna/visualization/contour.py 92.30% <100.00%> (+0.06%) ⬆️
...ration_tests/lightgbm_tuner_tests/test_optimize.py 99.45% <100.00%> (ø)

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 dc8207d...ec21fa7. Read the comment docs.

hvy
hvy reviewed Apr 30, 2020
View reviewed changes
optuna/pruners/hyperband.py Outdated
paper corresponds to ``max_resource / min_resource``. This value represents and should
match the maximum iteration steps (e.g., the number of epochs for neural networks).
When this argument is "auto", the maximum resource is estimated according to the
completed trials. The default value of this argument is "auto".
Copy link
Copy Markdown
Member

@hvy hvy Apr 30, 2020

Choose a reason for hiding this comment

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

With the last couple of sentences, this PR depends on #1171. Is it intentional?

@hvy hvy self-requested a review April 30, 2020 06:29
@hvy hvy mentioned this pull request Apr 30, 2020
Closed
@crcrpar crcrpar self-requested a review April 30, 2020 16:37
hvy
hvy reviewed May 1, 2020
View reviewed changes
hvy
hvy approved these changes May 1, 2020
View reviewed changes
Copy link
Copy Markdown
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, again LGTM!

crcrpar
crcrpar approved these changes May 1, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@crcrpar crcrpar left a comment

Choose a reason for hiding this comment

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

lgtm

@crcrpar crcrpar merged commit b2daf0d into optuna:master May 1, 2020
@HideakiImamura HideakiImamura mentioned this pull request May 27, 2020
Merged
@HideakiImamura HideakiImamura deleted the refactor/hyperband/fix_calc_n_brackets branch May 18, 2021 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hvy hvy hvy approved these changes

+1 more reviewer

@crcrpar crcrpar crcrpar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hvy hvy

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions.

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@HideakiImamura @nzw0301 @codecov-io @hvy @crcrpar