-
Notifications
You must be signed in to change notification settings - Fork 86
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
Making all tests pass with Implement_for functionality. Enables support most gym/gymnasium versions #119
Conversation
…st unpickling. Better handling for future gym versions
… versions - adding an helper util to prioritize gymnasium import over gym - standardizing gym_registry_specs for robohive env registration - moving all self.np_random.randint to self.np_random.choice as its more standard across versions - bugfix in range specifications. high-low<0 for a few instances - Adding a buffer (*_) in step and forward to accommodate different number of entities returned - starting to use env.unwrapped instead of env.env..... - robustifying test_logger. It can effectively fish out the log names now
@vmoens @Vittorio-Caggiano @0wu -- Can I get a round of reviews on this features? |
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
there seems to be a bit of unrelated changes for which I have little to say :)
Have you tried if the gym versions match? We may need to test if things work ok across a bunch of them
@@ -38,7 +38,7 @@ | |||
'model_path': curr_dir+'/trifinger/trifinger_reorient.xml', | |||
'object_site_name': "object", | |||
'target_site_name': "target", | |||
'target_xyz_range': {'high':[.05, .05, 0.9], 'low':[-.05, -.05, 0.99]}, | |||
'target_xyz_range': {'high':[.05, .05, 0.99], 'low':[-.05, -.05, 0.9]}, |
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 guess this is unrelated?
@@ -130,7 +133,7 @@ def _setup(self, | |||
self._setup_rgb_encoders(self.visual_keys, device=None) | |||
|
|||
# reset to get the env ready | |||
observation, _reward, done, _info = self.step(np.zeros(self.sim.model.nu)) | |||
observation, _reward, done, *_, _info = self.step(np.zeros(self.sim.model.nu)) |
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.
We could decorate this too but it's also ok if you don't need the truncated oc
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.
Seems to work for now. Lets revisit in the future incase this isn't sufficient
@@ -702,7 +728,7 @@ def examine_policy(self, | |||
ep_rwd = 0.0 | |||
while t < horizon and done is False: | |||
a = policy.get_action(o)[0] if mode == 'exploration' else policy.get_action(o)[1]['evaluation'] | |||
next_o, rwd, done, env_info = self.step(a) | |||
next_o, rwd, done, *_, env_info = self.step(a) |
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.
Ditto
@@ -794,7 +820,7 @@ def examine_policy_new(self, | |||
ep_rwd = 0.0 | |||
|
|||
# Rollout -------------------------------- | |||
obs, rwd, done, env_info = self.forward(update_exteroception=True) # t=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.
Ditto
@@ -825,7 +851,7 @@ def examine_policy_new(self, | |||
|
|||
|
|||
# step env using actions from t=>t+1 ---------------------- | |||
obs, rwd, done, env_info = self.step(act, update_exteroception=True) | |||
obs, rwd, done, *_, env_info = self.step(act, update_exteroception=True) |
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.
Ditto
…rom randint to choice
@vmoens -- I tried something like this - https://github.com/vikashplus/robohive/blob/implement_for/robohive/tests/test_versions.sh Any other salient versions we should check for? |
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.
Stamp for commit
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!
Nice work!
FEATURE: Adding support with passing tests for multiple gym/gymnasium versions