Skip to content
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

Mujoco 1.0 #733

Closed
wants to merge 6 commits into from
Closed

Mujoco 1.0 #733

wants to merge 6 commits into from

Conversation

adamlerer
Copy link

Some people have been asking for mujoco-1.0 (1.5.0.x) integration with gym (e.g. for headless rendering). This got it working for us.

@rhaps0dy
Copy link

Thank you, very useful! I'm using your code with the latest MuJoCo.

@machinaut machinaut self-requested a review October 21, 2017 17:26
high = bounds[:, 1]
self.action_space = spaces.Box(low, high)
bounds = self.model.actuator_ctrlrange
# I'm not sure why bounds is at least sometimes None ... bug?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamlerer / @yayitsamyzhang could you share examples of when this happens? Possibly a bug.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I've lost track of when this happened. You could try setting it to None and then run an environment and see what happens!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's constructed by generated code (here in the most recent version: https://github.com/openai/mujoco-py/blob/master/mujoco_py/generated/wrappers.pxi#L1285).

Wrapper is defined here: https://github.com/openai/mujoco-py/blob/master/mujoco_py/generated/wrappers.pxi#L3824

So it looks like we set it to None instead of a numpy array with 0 as one of the dimensions (which would just be empty anyways).

This would be true for models without any actuators (so all the ctrl related arrays are size 0).

I'm alright with this handling -- maybe update the comment to say that it's None when there are no actuators.

If you wanted to, you could check sim.model.nu == 0, which is the same thing (no actuators in the model).

Copy link
Contributor

@machinaut machinaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I'm looking forward to trying it out.

Given that this changes the underlying simulator, the environment name versions Humanoid-v1, etc need to be bumped, to show that the environment has changed.

If any of y'all are able to do it, it'd be good to try training policies for all of the old environments, and verify they work well on the new environments and vice versa.

self.model.forward()
sim_state = self.sim.get_state()
sim_state.qpos[:] = qpos
sim_state.qvel[:] = qvel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure what we want to do here is

self.data.qpos[:] = qpos
self.data.qvel[:] = qvel

The state is a separate thing, and if you set those parameters on it they'll just get thrown away.

@matthiasplappert
Copy link
Contributor

This is addressed in #834.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants