-
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
Add value function for torch #1317
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1317 +/- ##
=======================================
Coverage 91.50% 91.50%
=======================================
Files 218 220 +2
Lines 10970 10975 +5
Branches 1322 1324 +2
=======================================
+ Hits 10038 10043 +5
- Misses 675 676 +1
+ Partials 257 256 -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 is definitely a solid change in the right direction. Having said that, I think the ValueFunction
interface should be much smaller. Remember: it's easier to add new methods than to delete old ones.
Aside from that and some minor details, I'm quite happy with this change.
src/garage/torch/algos/maml.py
Outdated
@@ -63,7 +61,8 @@ def __init__(self, | |||
self._meta_evaluator = meta_evaluator | |||
self._policy = policy | |||
self._env = env | |||
self._value_function = value_function | |||
self._value_function = copy.deepcopy(inner_algo._value_function) | |||
self._lame_vf_state = self._value_function.state_dict() |
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.
Can you call it old
instead of lame
?
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.
@naeioi Can you check this?
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.
Conceptually it's the same, it's just that "lame" sounds a little unprofessional. Either way is okay, I guess.
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.
it's just confusing. use old
or previous
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.
Sorry I am late to the party. I made this name. This is not an old or previous value function, but a value function without any training. Now I think initial_vf_state
is the most appropriate name.
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 see. If it gets used in multiple places, I suppose I'm fine with it being fixed in a later PR. _initial_vf_state
is definitely a better name.
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.
Can you change this to _initial_vf_state
?
src/garage/torch/value_functions/gaussian_mlp_value_function.py
Outdated
Show resolved
Hide resolved
src/garage/torch/value_functions/gaussian_mlp_value_function.py
Outdated
Show resolved
Hide resolved
src/garage/torch/algos/trpo.py
Outdated
advs_flat) | ||
logger.log('Policy loss: {}'.format(policy_loss)) | ||
|
||
batch_dataset = BatchDataset((obs_flat, returns_flat), |
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.
why was this necessary for minibatching for the value function, but not the policy?
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 thought for TRPO, there is the cg_iters
field for ConjugateGradientOptimizer
and this works very similar with max_optimization_epochs
.
Is it okay to use minibatch for ConjugateGradientOptimizer?
If so, does this need to use same minibatch dataset but with different optimizing iterations? (cg_iters
and max_optimization_epochs
)?
Or it needs a single field for cg_iters
and max_optimization_epochs
?
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.
you can't use minibatching with ConjugateGradientOptimizer, or rather it would be kind of useless. Minibatching only works with SGD-based optimization algorithms, but a CG optimizer doesn't use SGD to choose parameters.
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.
Yes, so I used mini-batch only for value_function because default optimizer for value function is adam. When I run benchmark, I got a better result with minibatch and optimization_epochs
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.
what if i use the non-default ConjugateGradientOptimizer? will it still minibatch? wouldn't that be incorrect?
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.
In the tensorflow branch, we have our own optimizer class partly for this reason -- so that users can pass either a minibatching optimizer (which does batching inside of our own optimizer class), or the ConjugateGradientOptimizer
. The real solution for us is probably to do the same for the pytorch branch, but that seems like significant additional complexity for this change. I think the simplest change which wouldn't be wrong would be to not use minibatching with Adam for the time being (even though that makes performance worse), and open an issue to create a minibatching adam optimizer for pytoch in another PR.
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 saw in the CHANGELOG that Torch 1.5 started making it easier to add custom optimizers, but didn't look in detail.
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.
Great PR. See my comments and KR's (we are in agreement).
Overall this is like 80% there, but the intention of this PR is actually to totally remove the ValueFunction as a "special" primitive and make it act just like any other network in garage, so "predict" should not be necessary (for instance).
this will allow us to put the ValueFunction in the computation graph of the algorithm, which allows us to make a lot of things cleaner, and also enables us to do some interesting things.
What do the MuJoCo3M benchmarks look like? |
a0d74c2
to
7fbc0a4
Compare
7fbc0a4
to
83122d9
Compare
83122d9
to
f00e031
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.
Overall, looks good to me. I still think we should avoid using the term "lame," since it's unprofessional.
f00e031
to
a2207ee
Compare
src/garage/torch/algos/vpg.py
Outdated
@@ -432,7 +483,7 @@ def process_samples(self, itr, paths): | |||
for path in paths: | |||
if 'returns' not in path: | |||
path['returns'] = tu.discount_cumsum(path['rewards'], | |||
self.discount) | |||
self.discount).copy() |
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.
why copy this?
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.
If not, I got the error
E ValueError: some of the strides of a given numpy array are negative. This is currently not supported, but will be added in future releases.
This is becaue we revert the order of output [::-1]
in the discount_cumsum
function.
So I need to copy the array
You can also check here
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.
How did it work before? What changed?
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.
Can you use some other numpy function to get a reversed view, other than negative indexing?
When you call copy()
you break the gradient path. If your returns have a differentiable components, e.g. a differentiable reward augmentation, your augmented rewards will no longer be differentiable.
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 error is from here.
Previously, there is only LinearFeatureBaseline which doesn't have to convert numpy.array to torch.tensor. But now we need to convert numpy.array to torch.tensor for GaussianMLPValueFunction.
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.
According to this post, problem is in PyTorch. PyTorch doesn't support numpy.array with negative stride.
This is one solution not using .copy()
torch.Tensor( tu.discount_cumsum(path['rewards'], self.discount) [::-1] ).flip(-1)
For the gradient path, the returns values from the tu.discount_cumsum
is numpy.array not a torch.tensor, so I think there is no gradient path for the values yet.
Also I'm just curious that we use rewards from environment to compute the return values and there is module or functions that compute gradient for the rewards?
I added new OptimizerWrapper class. Overall process is
This PR contains many changes, so I will put changes only for OptimizerWrapper |
c1a5687
to
4326ba8
Compare
@naeioi Could you check the changes and how they affect MAML algorithm? @krzentner Could you check this PR for me? Thanks! |
@@ -264,49 +247,3 @@ def run_garage_tf(env, seed, log_dir): | |||
dowel_logger.remove_all() | |||
|
|||
return tabular_log_file | |||
|
|||
|
|||
def run_baselines(env, seed, log_dir): |
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.
Do we want to delete this? This should be the only place we could locate the baselines benchmarking code.
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.
It looks like the openai.baseline version is not support Tensorflow 2.0.
@@ -218,59 +215,3 @@ def run_garage(env, seed, log_dir): | |||
dowel_logger.remove_all() | |||
|
|||
return tabular_log_file | |||
|
|||
|
|||
def run_baselines(env, seed, log_dir): |
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.
Same here
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.
It looks like the openai.baseline version is not support Tensorflow 2.0.
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. This is an important change! Please fix my minor comment
src/garage/torch/algos/maml.py
Outdated
@@ -63,7 +61,8 @@ def __init__(self, | |||
self._meta_evaluator = meta_evaluator | |||
self._policy = policy | |||
self._env = env | |||
self._value_function = value_function | |||
self._value_function = copy.deepcopy(inner_algo._value_function) | |||
self._lame_vf_state = self._value_function.state_dict() |
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.
Can you change this to _initial_vf_state
?
4326ba8
to
2146c20
Compare
2146c20
to
d6316d4
Compare
d6316d4
to
515f897
Compare
@Mergifyio rebase |
Command
|
515f897
to
3bcb7c2
Compare
3bcb7c2
to
89a1ff9
Compare
@Mergifyio rebase |
Command
|
Implement GaussianMLPValueFunction for PyTorch algorithms.
A value function computes loss and returns the value to an algorithm to update the weights.
An algorithm has two optimizers, for policy and value function.