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
Add HyperbandPruner
#809
Add HyperbandPruner
#809
Conversation
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
=========================================
Coverage ? 90.13%
=========================================
Files ? 108
Lines ? 8914
Branches ? 0
=========================================
Hits ? 8035
Misses ? 879
Partials ? 0
Continue to review full report at Codecov.
|
0921c30
to
584286b
Compare
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.
Thank you for the swift update. The current design looks good for me.
By the way, I left some minor comments about the code detail.
optuna/pruners/hyperband.py
Outdated
return self._n_pruners | ||
|
||
|
||
class _BracketStudy(Study): |
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.
Other methods besides get_trials
might behaive unexpectedly as they are not filtered. I believe we can either override and raise, or avoid inheriting from Study
.
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 tried to disable other methods of Study
in a naive way. PTAL.
As to (This is the clarification as the discussion is getting long and not accessible.) |
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 with nits!! I think current implementation is reasonable for now 👍 Good job @crcrpar.
Discussed how to compute budgets and allocate trials to brackets with @sile, and one way is to merge this without any modification because
I don't mean we should stop here, merge first without further improvement. From the perspective of the granularity and duration of the PR, merging is one option. |
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 all for the review and discussion.
Though this PR satisfies the condition to get merged, I'd like to add TODO
comment about resource allocation to brackets.
Thank you for your understanding beforehand.
…On Thu, Jan 9, 2020 at 18:10 Hiroyuki Vincent Yamazaki < ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#809?email_source=notifications&email_token=AD3Q7U5FA7PXESFAHYK5ESTQ43SZ5A5CNFSM4J4IZEZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCRE3OIY#pullrequestreview-340375331>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3Q7UY55TF64IPMV3GGTOTQ43SZ5ANCNFSM4J4IZEZA>
.
|
Added the TODO. |
Thanks! |
Thanks for this implementation! It's awsome! :-) |
This depends on #808The algorithm would behave differently from the paper if the sampler is
TPESampler
because there is no way to filter out the trials of brackets other than that of the running trial.Although #785 and #805 are for Hyperband, this PR follows another design policy.The core design of #785 is to realize filtering by adding an attribute that represents pruner to Trial.
It will make
Study
more complex while it can keep the current loosely connected relationship betweenSampler
andPruner
.On the other hand, the policy of this PR does not implement any filtering of trials.This means that we will need to add some interaction between
HyperbandPruner
andSampler
, especiallyTPESampler
.EDIT:
In this implementation, trials are filtered by
bracket_id
that is computed byHyperbandPruner._get_bracket_id
and this filtering is enabled by implementing_BracketStudy
which just wrapsStudy
for theHyperbandPruner
use only. The idea is suggested in #809 (comment).