-
Notifications
You must be signed in to change notification settings - Fork 310
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
New optimizer_args tf interface + test optimizer type #1496
Conversation
benchmarks/src/garage_benchmarks/experiments/algos/ppo_garage_tf.py
Outdated
Show resolved
Hide resolved
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.
See my comments
Awesome work! |
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.
Good job! The semantic of make_optimizer()
is a bit off from the torch version. It should take parameters from both user and defaults. Please take a look at how torch implements it.
96c9887
to
b655ff8
Compare
benchmarks/src/garage_benchmarks/experiments/algos/ppo_garage_tf.py
Outdated
Show resolved
Hide resolved
I push the unified tf and PyTorch optimizer interface not long ago. There are pickling tests i.e. A sample of the error in Meanwhile, I'll address the rest of the comments. |
b655ff8
to
d0233b5
Compare
Be careful with merge commits! They really mess with our CI (which only assumes rebases). Only use |
394b5fb
to
977fb80
Compare
I resolved the python pickle issues on the tf algorithm when using
|
Hmm -- can |
a867c2b
to
d066f5f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1496 +/- ##
=======================================
Coverage 90.81% 90.81%
=======================================
Files 211 211
Lines 10334 10346 +12
Branches 1262 1261 -1
=======================================
+ Hits 9385 9396 +11
Misses 699 699
- Partials 250 251 +1
Continue to review full report at Codecov.
|
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.
This looks very good to me. Please address my tiny comments before submit.
d066f5f
to
c8b41c5
Compare
c8b41c5
to
bdbf0fa
Compare
This PR addresses issue #1062
optimizer_args
pattern from tf algorithm constructors intf/algos
,tf/optimizers
andtf/regressors
,The tf optimizer interface
garage/tf/algos/_utils.py
follows the compatibility of TensorFlow Core v2.2.0 and is similar to torch's optimizer interface but accepts an optimizer type only. The algorithm internally constructs an optimizer using a new utility function_make_optimizer
.