-
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
Sampler should not flatten observations and actions #967
Conversation
Codecov Report
@@ Coverage Diff @@
## master #967 +/- ##
==========================================
+ Coverage 84.49% 84.58% +0.09%
==========================================
Files 156 156
Lines 7467 7468 +1
Branches 938 938
==========================================
+ Hits 6309 6317 +8
+ Misses 966 964 -2
+ Partials 192 187 -5
Continue to review full report at Codecov.
|
6a4f23d
to
2582809
Compare
src/garage/sampler/utils.py
Outdated
@@ -111,6 +115,6 @@ def truncate_paths(paths, max_samples): | |||
truncated_last_path[k] = tensor_utils.truncate_tensor_dict( | |||
v, truncated_len) | |||
else: | |||
raise NotImplementedError | |||
raise NotImplementedError() |
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 raise a ValueError instead? and include an error message which specifies the valid keys versus the ones found?
Not directly related to this PR, but there is also inconsistency in flattening in primitives. For example,
while categorical_cnn_policy.py does not
To make things even worse, there is a garage/src/garage/tf/algos/batch_polopt.py Line 145 in dbec2b6
We should make all these into a single switch for flattening. |
@naeioi @ryanjulian Yes I think right now flattening is duplicate in both primitive and algorithm - we should probably only do that in one place (which I think we chose to be algorithm). That said, we should remove the flattening in all primitives and add flattening support in all other algorithms (except Regarding |
@ahtsan that sounds right to me. for now let's just make it consistent and we can consider more automatic designs later. |
I added some tests to try to merge this for the release, but it turns out something is still quite broken (or my tests are just wrong). In any case, this won't make it and we will have to backport the fix I think. @ahtsan @krzentner should be interested. |
2582809
to
10f7c4f
Compare
I think the issue is that the dummy policy doesn't have dist_info_keys to extract agent_info, and the test assumes "env_info is not empty (which is not true in reality) and agent_info is not empty". We should probably fix the dummy policy and make them inherit the policy base class to make sure they comply with the APIs. We might also want to introduce I am working on a fix. |
If you look at the broken test, I think the shape of |
Do you mean this test doesn't pass? def test_does_not_flatten(self):
path = utils.rollout(self.env, self.policy, max_path_length=5)
assert path['observations'][0].shape == (4, 4)
assert path['actions'][0].shape == (2, 2) |
Yes |
0bfcc0e
to
1ca6f8d
Compare
1ca6f8d
to
88727b5
Compare
186eaaa
to
8789f0b
Compare
To make API consistent among all samplers, samplers should not flatten the observation nor the actions, which is done by algorithms.