Skip to content

Fix improper stopping with the combination of GridSampler and HyperbandPruner#2353

Merged
HideakiImamura merged 3 commits intooptuna:masterfrom
not522:fix-2327
Feb 18, 2021
Merged

Fix improper stopping with the combination of GridSampler and HyperbandPruner#2353
HideakiImamura merged 3 commits intooptuna:masterfrom
not522:fix-2327

Conversation

@not522
Copy link
Copy Markdown
Member

@not522 not522 commented Feb 16, 2021

Motivation

Resolve #2327

Description of the changes

When we use HyperbandPruner, study.trials is filtered by bracket_id. This causes improper stopping bug for GridSampler because it cannot check all nodes are visited using study.trials.
This PR adds stop method for _BracketStudy and checks visited nodes using study._storage.get_all_trials.

@github-actions github-actions bot added optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. labels Feb 16, 2021
@not522 not522 added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Feb 16, 2021
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 16, 2021 •
edited
Loading

Codecov Report

Merging #2353 (4fdd146) into master (6be61bb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2353      +/-   ##
==========================================
+ Coverage   92.22%   92.23%   +0.01%     
==========================================
  Files         125      125              
  Lines       10344    10385      +41     
==========================================
+ Hits         9540     9579      +39     
- Misses        804      806       +2
Impacted Files Coverage Δ
optuna/pruners/_hyperband.py 98.80% <100.00%> (+0.04%) ⬆️
optuna/samplers/_grid.py 98.82% <100.00%> (+0.01%) ⬆️
optuna/storages/_rdb/storage.py 92.73% <0.00%> (-0.22%) ⬇️
optuna/integration/pytorch_lightning.py 87.50% <0.00%> (ø)
optuna/integration/_lightgbm_tuner/__init__.py 69.23% <0.00%> (ø)
optuna/storages/_cached_storage.py 97.04% <0.00%> (+0.03%) ⬆️
optuna/_optimize.py 97.83% <0.00%> (+0.07%) ⬆️
optuna/storages/_base.py 68.93% <0.00%> (+0.18%) ⬆️
optuna/samplers/_cmaes.py 97.64% <0.00%> (+0.19%) ⬆️
optuna/integration/tensorboard.py 89.79% <0.00%> (+2.04%) ⬆️

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 6be61bb...4fdd146. Read the comment docs.

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented Feb 17, 2021

@toshihikoyanase Could you review this PR?

HideakiImamura
HideakiImamura reviewed Feb 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Look solid. I have a minor comment on the code comment. Please take a look.

@not522 @HideakiImamura
Update optuna/samplers/_grid.py
4fdd146 
Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>
toshihikoyanase
toshihikoyanase reviewed Feb 17, 2021
View reviewed changes
Copy link
Copy Markdown
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 fixing the bug. The code looks solid, but I added some comments on the tests.

@not522 @toshihikoyanase
Apply suggestions from code review
79508b4 
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
@not522
Copy link
Copy Markdown
Member Author

not522 commented Feb 17, 2021

Thank you for your reviews. I applied your suggestions. PTAL.

toshihikoyanase
toshihikoyanase approved these changes Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
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. Looks good to me.

HideakiImamura
HideakiImamura approved these changes Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura 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 the quick fix! LGTM!

@HideakiImamura HideakiImamura merged commit b04e704 into optuna:master Feb 18, 2021
@HideakiImamura HideakiImamura added this to the v2.6.0 milestone Feb 18, 2021
@not522 not522 deleted the fix-2327 branch May 6, 2021 08:56
@himkt himkt mentioned this pull request May 31, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toshihikoyanase toshihikoyanase toshihikoyanase approved these changes

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@toshihikoyanase toshihikoyanase

@HideakiImamura HideakiImamura

Labels

bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.pruners Related to the `optuna.pruners` submodule. This is automatically labeled by github-actions. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions.

Projects

None yet

Milestone

v2.6.0

Development

Successfully merging this pull request may close these issues.

Study.optimize cannot be stopped properly when sampler=GridSampler and pruner=HyperbandPruner

4 participants

@not522 @codecov-io @HideakiImamura @toshihikoyanase