[tune] Enable list of tuning hyperparameters in BOHB #14487
Conversation
python/ray/tune/utils/util.py
Outdated
| @@ -298,6 +298,32 @@ def unflatten_dict(dt, delimiter="/"): | |||
| return out | |||
|
|
|||
|
|
|||
| def unflatten_list_dict(dt, delimiter="/"): | |||
| """Unflatten nested dict and list.""" | |||
| out_type = list if list(dt)[0].split(delimiter, 1)[0].isdigit() else dict | |||
There was a problem hiding this comment.
I think we can add a comment explaining what this does and why we need it?
also, I think this will raise an error if dt is {3: "is an int"} because list(dt)[0] is an int, which doesn't have split
There was a problem hiding this comment.
You are right! This will raise an error in your example.
But I think this function now is only for one purpose: correctly unflatten config. And as far as I know all the flattened config is a dict, and its keys are all str.
Therefore, your example may not appear. This will be confirmed (or not) by unit test.
Actually I think writing such a function with strong robustness isn't that easy. So I agree with adding a comment explaining what this does, especially its limitation. And perhaps we need to add more test?
There was a problem hiding this comment.
I've added a comment in the latest commit :)
Co-authored-by: Sumanth Ratna <sumanthratna@gmail.com>
ghost
requested a review
from 
There was a problem hiding this comment.
Hi @RaphaelCS , thank you for this contribution! Could you add a couple unit tests in tune/tests/test_trainable_util::UnflattenDictTest ?
Yes, I can try. But I notice that Travis CI failed. We need to fix it first, right? |
|
Don’t worry, it’s an unrelated issue!
…On Fri, Mar 5, 2021 at 6:36 PM Raphael CHEN ***@***.***> wrote:
Hi @RaphaelCS <https://github.com/RaphaelCS> , thank you for this
contribution! Could you add a couple unit tests in
tune/tests/test_trainable_util::UnflattenDictTest ?
Yes, I can try.
But I notice that Travis CI failed. We need to fix it first, right?
If it is, could anyone help me locate which test failed? (I'm not familiar
with Travis CI 😢 ) I can also try to fix it if I know which test failed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14487 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCRZZK3FLDUHITCTZOXTETTCGIJZANCNFSM4YSPICJA>
.
|
Ah, OK. So I'll try to add some unit tests in a few days 😄 |
Why are these changes needed?
BOHB can now deal with list (and dict) of hyperparameters.
Related issue number
Closes #14472
Checks
scripts/format.shto lint the changes in this PR.