-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Tune] Support initial parameters for SkOpt search algorithm #4341
[Tune] Support initial parameters for SkOpt search algorithm #4341
Conversation
cc @adizim, can you review this PR? It is based off of your previous contribution to the SkOpt search algorithm component. |
Can one of the admins verify this patch? |
I left a few suggestions. I see you have tested the example case with points_to_evaluate and evaluated_rewards set. Similarly, can you see that the example without evaluated_rewards set works, to cover all cases? |
BTW @markgoodhead, thanks for this contribution!
BTW, this should work soon, and the relevant PRs should be #4234 and #3709. |
Test PASSed. |
I wrote up my suggestions in #4345 |
Hi @adizim - thanks for the review! I'm a little confused as to what branch you'd like me to implement the changes off as #4345 has been closed and I also see you made a PR into my branch (which contains a lot of other changes besides your tweaks so I think muddies the PR too much) - what way would you like me to proceed from here? EDIT: Never mind I worked it out that the other changes are just due to me being very behind master - I've merged your branch and will do the other changes you suggest 😄 EDIT2: OK actually it looks like there's some other changes in there that's unrelated to this PR @adizim - mainly in python/ray/rllib/tests/test_lstm.py - are you working on these changes elsewhere that need to be merged into master first? Are you happy for them to go in via my branch? |
Update on support initial parameters for SkOpt
Test FAILed. |
…the skopt example to run both with and without known_rewards
@richardliaw @adizim I've made all the requested changes after merging @adizim 's branch. This PR now has some random RLLib changes in so not sure what you want to do about that but otherwise it's good to go. |
Test FAILed. |
@markgoodhead Apologies for the extraneous changes, I did not mean to include them, I hope they didn't cause too much confusion. @richardliaw was able to revert them just recently. @markgoodhead The changes look good to me, though it would be helpful to keep the error messages you wrote. Raising TypeErrors with the error messages should be the correct approach. You can also include the error messages in the assertions as they stand now. |
@markgoodhead Nevermind, @richardliaw took care of the error messages |
jenkins retest this please |
Test PASSed. |
Test FAILed. |
jenkins retest this please |
Test PASSed. |
Test FAILed. |
…ed to length of parameter_names (number of parameter dimensions). Fixed this comparion and also fixed the points_to_evaluate and evaluated_rewards comparison
Test FAILed. |
Why do the tests keep failing? |
I think one of the new tests hang very often and there’s a PR to fix it,
should be merged soon.
I’ll just run the Skopt test locally and merge if passing though.
…On Fri, Mar 15, 2019 at 3:02 PM markgoodhead ***@***.***> wrote:
Why do the tests keep failing?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#4341 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEUc5bNra3A4JL_B7mt9NaxmGZuZ1JRbks5vXBh3gaJpZM4brZ7O>
.
|
This runs locally. |
Test FAILed. |
What do these changes do?
Similar to the recent change to HyperOpt (##3944) this implements both: