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

[Bug Report] MujocoEnv only accepts frame_skip=5 #473

Closed
1 task done
Omer1Yuval1 opened this issue Apr 28, 2023 · 16 comments
Closed
1 task done

[Bug Report] MujocoEnv only accepts frame_skip=5 #473

Omer1Yuval1 opened this issue Apr 28, 2023 · 16 comments
Labels
bug Something isn't working

Comments

@Omer1Yuval1
Copy link

Omer1Yuval1 commented Apr 28, 2023

Describe the bug

The AntEnv class in ant-v4.py calls the MujocoEnv class with frame_skip=5 by default. Changing this value to any other value (e.g., frame_skip=1) results in an error.

More generally, it would be useful to be able to directly control the time step (self.dt). Currently it seems like the only way to control self.dt is by modifying self.frame_skip, such that self.dt=self.model.opt.timestep * self.frame_skip.

Code example

MujocoEnv.__init__(
    self,
    xml_file,
    frame_skip=1,
    observation_space=observation_space,
    default_camera_config=DEFAULT_CAMERA_CONFIG,
    **kwargs,
)

    MujocoEnv.__init__(
  File "D:\programs\Python\Python310\lib\site-packages\gymnasium\envs\mujoco\mujoco_env.py", line 333, in __init__
    super().__init__(
  File "D:\programs\Python\Python310\lib\site-packages\gymnasium\envs\mujoco\mujoco_env.py", line 65, in __init__
    int(np.round(1.0 / self.dt)) == self.metadata["render_fps"]
AssertionError: Expected value: 100, Actual value: 20

System info

No response

Additional context

No response

Checklist

  • I have checked that there is no similar issue in the repo
@Omer1Yuval1 Omer1Yuval1 added the bug Something isn't working label Apr 28, 2023
@pseudo-rnd-thoughts
Copy link
Member

pseudo-rnd-thoughts commented Apr 30, 2023

Thanks for reporting this, @rodrigodelazcano any idea of what we should do?
We could change the error to a warning that tells people that this affects the render_fps

@younik Could we add a fps parameter to the RecordVideoV0 to let people choose a custom fps that is not the environment's render_fps. It can default to 30 to keep backward compatibility

@younik
Copy link
Member

younik commented Apr 30, 2023

@younik Could we add a fps parameter to the RecordVideoV0 to let people choice a custom fps that is not the environment's render_fps. It can default to 30 to keep backward compatibility

Sure; for backward compatibility, the default value should be None and in that case, we use the value in metadata, correct?

@Kallinteris-Andreas
Copy link
Collaborator

@pseudo-rnd-thoughts close?

@Omer1Yuval1
Copy link
Author

From what I understood, in order to change the frame rate you would have to change both render_fps and frame_skip, such that frame_skip = 100 / render_fps (I followed what the error said, but it's not clear to me why it should be this way). Here is a simple example:

dt = 0.01 # Simulation time step in seconds.
render_fps = round(1 / dt)
frame_skip = round(100 / render_fps)

Can the developers please confirm that this makes sense?

@pseudo-rnd-thoughts
Copy link
Member

@Omer1Yuval1 The render_fps is independent from the number of actions taken per simulation second and can in theory be any value. But round(1000/dt) seems reasonable, you need a positive integer.
For the frame_skip, this is more complex as I believe that this refers to the length of internal time the same action is taken and should definitely be separate from render_fps.

@Kallinteris-Andreas No, as the original error of modifying frame_skip has no been solved. @rodrigodelazcano I think you missed the last @, could you look at the original error and know if there is an easy solution

@Kallinteris-Andreas
Copy link
Collaborator

The current implementation, does not support changing the render_fps, the metadata is a static global variable (treated as a constant)

$ py 
Python 3.11.3 (main, Apr  5 2023, 15:52:25) [GCC 12.2.1 20230201] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import gymnasium
>>> env = gymnasium.make('Ant-v4')
>>> env.metadata
{'render_modes': ['human', 'rgb_array', 'depth_array'], 'render_fps': 20}
>>> env.metadata['render_fps'] = 40
>>> env = gymnasium.make('Ant-v4')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/master-andreas/.local/lib/python3.11/site-packages/gymnasium/envs/registration.py", line 801, in make
    env = env_creator(**env_spec_kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/master-andreas/.local/lib/python3.11/site-packages/gymnasium/envs/mujoco/ant_v4.py", line 267, in __init__
    MujocoEnv.__init__(
  File "/home/master-andreas/.local/lib/python3.11/site-packages/gymnasium/envs/mujoco/mujoco_env.py", line 333, in __init__
    super().__init__(
  File "/home/master-andreas/.local/lib/python3.11/site-packages/gymnasium/envs/mujoco/mujoco_env.py", line 65, in __init__
    int(np.round(1.0 / self.dt)) == self.metadata["render_fps"]
AssertionError: Expected value: 20, Actual value: 40

@Kallinteris-Andreas
Copy link
Collaborator

Kallinteris-Andreas commented May 24, 2023

@pseudo-rnd-thoughts as far as I can tell it is not a violation of the gymansium.Env API that different instances of the same environment can have different metadata. Is that the case?

Also, (from a quick look) it does not appear that anything would break.

@pseudo-rnd-thoughts
Copy link
Member

@Kallinteris-Andreas Yeah, I should be fine to update the metadata of the environment based on arguments

@Kallinteris-Andreas
Copy link
Collaborator

@pseudo-rnd-thoughts Great, I will add the frame_skip arg to the mujoco-v5 environments, and compute metadata.render_fps on __init__ time

@Kallinteris-Andreas
Copy link
Collaborator

Kallinteris-Andreas commented May 26, 2023

@Omer1Yuval1 just set metadata['render_fps"] = 100, to use it with Ant-v4

Note: this issue will be fixed in v5

>>> import gymnasium
>>> env = gymnasium.make('Ant-v5', frame_skip=1)

@Kallinteris-Andreas
Copy link
Collaborator

  1. yes you need to adjust the metadata["render_fps"]
  2. ~
    2.1 the action represent the control forces applied to the robot for self.dt duration (an env step), the environment does not apply impulsive forces.
    2.2 I do not understand what the problem in the documentation is.
    2.3 what render_mode are you using, does it work without a render_mode=None
  3. Define "spiky actions" and "exaggerated joint angles"

@Omer1Yuval1
Copy link
Author

Omer1Yuval1 commented Jun 3, 2023

Thanks everyone for your replies and insights. I have a few follow-up questions.

  1. render_fps

    The render_fps is independent from the number of actions taken per simulation second and can in theory be any value. But round(1000/dt) seems reasonable, you need a positive integer.

    @pseudo-rnd-thoughts Do I understand correctly that currently render_fps is dependent on dt in the code and must be adjusted in order to run it without errors?

  2. frame_skip, dt and frametime
    According to the documentation inside the ant_v4.py file

    dt is the time between actions and is dependent on the frame_skip parameter (default is 5), where the frametime is 0.01 - making the default dt = 5 * 0.01 = 0.05

    This is done here in mujoco_env.py

    @property
    def dt(self):
        return self.model.opt.timestep * self.frame_skip
    

    If I understand correctly frametime refers to self.model.opt.timestep (set in ant.xml). Can you please clarify the physical meaning and difference between self.model.opt.timestep (defined in ant.xml) and self.dt?

@Kallinteris-Andreas
Copy link
Collaborator

  1. yes you need to adjust the metadata["render_fps"]
  2. @property
    def dt(self):
    return self.model.opt.timestep * self.frame_skip

@pseudo-rnd-thoughts
Copy link
Member

yes you need to adjust the metadata["render_fps"]

I might be wrong but to modify the render_fps value will require you to locally install gymnasium, modify the value then pip install . within gymnasium's root. Or just wait for the next minor release which we are aiming for the next two or so weeks (no promises)

@Omer1Yuval1
Copy link
Author

@Kallinteris-Andreas in your reply you quoted the same piece of code that I mentioned in my question. I am trying to understand the meaning/implications of having a different time step for the MuJoCo engine and environment solvers.

@Kallinteris-Andreas
Copy link
Collaborator

Having a bigger dt means there will be more information contained in a step() (s, a, r, s') and therefore get your learning algorithm to learn with higher sample efficiency

The downside is the inability to give learn finer control

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants