Skip to content

Add trials to *Sampler.sample_{independent, relative}#805

Closed
crcrpar wants to merge 2 commits intooptuna:masterfrom
crcrpar:trials-to-samplers
Closed

Add trials to *Sampler.sample_{independent, relative}#805
crcrpar wants to merge 2 commits intooptuna:masterfrom
crcrpar:trials-to-samplers

Conversation

@crcrpar
Copy link
Copy Markdown
Contributor

@crcrpar crcrpar commented Dec 18, 2019

Context: This PR derives from #785.
#785 is not trivial and composed of two major changes: hyperband implementation and new argument.
But the changes related to the new argument is independent from hyperband implementation.

This PR changes BaseSampler.sample_{independent, relative} to take trials, a list of trials as an argument.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 18, 2019

Codecov Report

Merging #805 into master will decrease coverage by 0.06%.
The diff coverage is 82.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
- Coverage   90.15%   90.09%   -0.07%     
==========================================
  Files         106      106              
  Lines        8769     8793      +24     
==========================================
+ Hits         7906     7922      +16     
- Misses        863      871       +8
Impacted Files Coverage Δ
optuna/study.py 93.62% <100%> (+0.1%) ⬆️
optuna/integration/cma.py 94.03% <100%> (+0.08%) ⬆️
optuna/integration/skopt.py 88.42% <100%> (ø) ⬆️
tests/test_trial.py 99.11% <100%> (ø) ⬆️
optuna/samplers/tpe/sampler.py 87.54% <100%> (ø) ⬆️
optuna/integration/lightgbm_tuner/optimize.py 76.03% <100%> (ø) ⬆️
optuna/samplers/base.py 47.36% <50%> (+0.7%) ⬆️
tests/samplers_tests/test_samplers.py 91.53% <66.66%> (-0.71%) ⬇️
optuna/samplers/random.py 83.33% <66.66%> (-1.78%) ⬇️
optuna/testing/sampler.py 60% <66.66%> (-3.16%) ⬇️
... and 1 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 0389962...d91a2e7. Read the comment docs.

@crcrpar crcrpar mentioned this pull request Dec 18, 2019
Closed
@crcrpar
Copy link
Copy Markdown
Contributor Author

crcrpar commented Dec 18, 2019

Summary of off-line discussion with @sile

  • Current design is too ad hoc given that
    • Not parallel with pruners
    • SuccessiveHalvingPruner (SHA) does the similar thing with rung_key_prefix, but it's only implemented in SHA. However, the changes mainly for HyperbandPruner have an effect on all the samplers
    • A new attribute for pruner information is also ad hoc. It's registered as a user_attr

-> Need more generalized design/implementation from that other coming/existing pruners/samplers can benefit.

Design Idea

A. Inspired by Ax somewhat

Study manages Arms. All the trials must belong to one of them.
By default, trials belong to the default arm.
And each trial has the information of which arm it belongs to.
This information should be included in FrozenTrial.
Study.get_trials takes a new argument arm and collects trials in the arm.
If a trial needs to be registered to another arm, the study sets the trial via Trial.set_arm.
In Trial._suggest method, call study.get_trials not study.trials to collect appropriate trials.

This was referenced Dec 18, 2019
Closed
Merged
@crcrpar
Copy link
Copy Markdown
Contributor Author

crcrpar commented Dec 25, 2019

Let me close this PR as I think #809 is enough to implement Hyperband.

@crcrpar crcrpar closed this Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@crcrpar @codecov-io