adding reverse openai gym wrapper and tests #8
Conversation
There was a problem hiding this comment.
Thanks for the PR! A few suggestions (mostly style nits). Many of these can be resolved automatically if you run pyformat over the code -- see https://pypi.org/project/pyformat/ for instructions.
bsuite/utils/gym_wrapper.py
Outdated
|
|
||
|
|
||
| class ReverseGymWrapper(dm_env.Environment): | ||
| """A wrapper that converts an OpenAI gym environment to a dm_env.Environment""" |
There was a problem hiding this comment.
Looks like you're using 4 spaces here. Are you able to switch to 2 to be consistent with the rest of the codebase?
bsuite/utils/gym_wrapper.py
Outdated
| class ReverseGymWrapper(dm_env.Environment): | ||
| """A wrapper that converts an OpenAI gym environment to a dm_env.Environment""" | ||
|
|
||
| def __init__(self, gym_env:gym.Env): |
There was a problem hiding this comment.
Type annotations need a space after the ':' (ditto elsewhere)
bsuite/utils/gym_wrapper.py
Outdated
| def step(self, action): | ||
| """convert gym step result (observations, reward, done, info) to dm_env TimeStep""" | ||
| gym_step_res = self.gym_env.step(action) | ||
| _obs = gym_step_res[0] |
There was a problem hiding this comment.
Also it's more idiomatic to do something like
obs, reward, done, _ = self.gym_env.step(action)
bsuite/utils/gym_wrapper.py
Outdated
| self.gym_env.reset() | ||
|
|
||
| def step(self, action): | ||
| """convert gym step result (observations, reward, done, info) to dm_env TimeStep""" |
There was a problem hiding this comment.
Can you make your docstrings proper sentences, i.e. 'Convert .... TimeStep.' (ditto elsewhere)
bsuite/utils/wrappers_test.py
Outdated
|
|
||
| import dm_env | ||
| from dm_env import specs | ||
| from dm_env import test_utils | ||
| import mock | ||
| import numpy as np | ||
|
|
||
| import gym |
There was a problem hiding this comment.
Imports should be alphabetical, so this should be after dm_env and before mock.
bsuite/utils/wrappers_test.py
Outdated
|
|
||
| # test converted observation spec | ||
| self.assertEqual(type(self.gym_test_env.observation_spec()), specs.BoundedArray) | ||
| self.assertEqual(self.gym_test_env.observation_spec().shape,(4,)) |
There was a problem hiding this comment.
Can you add a single space after commas?
bsuite/utils/gym_wrapper.py
Outdated
| @@ -59,6 +59,7 @@ def render(self, mode: Text = 'rgb_array') -> Union[np.ndarray, bool]: | |||
| if mode == 'human': | |||
| if self.viewer is None: | |||
| from gym.envs.classic_control import rendering # pylint: disable=g-import-not-at-top | |||
|
|
|||
bsuite/utils/gym_wrapper.py
Outdated
| def action_spec(self): | ||
| return self._action_spec | ||
|
|
||
| def space2spec(self, space:gym.Space, name:str=None): |
There was a problem hiding this comment.
Does this need to be a public method?
There was a problem hiding this comment.
It should also be a function, rather than a method.
bsuite/utils/wrappers_test.py
Outdated
| class ReverseGymWrapperTest(absltest.TestCase): | ||
|
|
||
| def test_gym_cartpole(self): | ||
| self.gym_test_env = gym_wrapper.ReverseGymWrapper(gym.make('CartPole-v0')) |
There was a problem hiding this comment.
There's no need to assign anything to self here. Can we just have a local gym_env variable?
|
Thanks for the comments @aslanides and @yotam , I should be able to make the corrections in the next day or so. |
|
@yotam @aslanides I made some changes based on your suggestions |
|
Thanks for the changes Alex. I'm going to merge the change internally then push it and close here. Will make a few further edits and formatting changes on the way. |
|
Merged in af5b3fe |
|
Thanks @yotam |
This PR is related to #6 where I suggested a wrapper that converts Openai gym environments to bsuite environments. This PR contains the wrapper and a test case that converts the Openai CartPole-v0 environment to a bsuite environment.