Jt/port - #40
Conversation
| from gym import LEGGED_GYM_ROOT_DIR | ||
| from gym.envs.base.sim_backend import SimBackend | ||
|
|
||
| from gym.envs.mini_cheetah.mini_cheetah_config import MiniCheetahCfg |
There was a problem hiding this comment.
we don't want to add code specific to a single robot to a file that should be general across the entire framework. Instead, we should set default value or read the values from the config if they exist.
| c[:, 1] = torch.clamp(c[:, 1] + self.increment_y, max=self.max_vel_sideways) | ||
| elif keycode == KEY_PERIOD: | ||
| c[:, 1] = torch.clamp(c[:, 1] - self.increment_y, min=-self.max_vel_sideways) | ||
| c[:, 1] = torch.clamp( |
| dependencies = [ | ||
| "mujoco>=3.6", | ||
| "torch", | ||
| "torch>=2.0", |
There was a problem hiding this comment.
was this needed? Or what was the issue?
sheim
left a comment
sheim
left a comment
There was a problem hiding this comment.
Some issues, especially with things implemented in the environment directly instead of through configs. See comments.
| super().__init__( | ||
| gym, sim, cfg, sim_params, sim_device, headless, backend=backend | ||
| ) | ||
| self.states_to_log = [ |
There was a problem hiding this comment.
We don't want to have specific things like this in the environment class, this leads to having to update code ad hoc, instead of changing things in the config file. The point is to use the getters/setters, so that the environment implementation is agnostic (unaware) of any details like how the policy is implemented, how plotting/logging is handled etc.
| import torch | ||
|
|
||
| from gym.envs.base.legged_robot import LeggedRobot | ||
| # from learning.utils.logger.SaveStates import ( |
|
|
||
| self.root_states[...] = ( | ||
| self._root_states_t | ||
| ) # fixes root_states from not being updated |
| spec = self._load_urdf_spec(asset_path) | ||
| spec.compiler.balanceinertia = True | ||
|
|
||
| # Disable fusing links connected with rigid joints |
There was a problem hiding this comment.
Clean Code: avoid comments, they get stale and obsolete and eventually lead to more confusion than clarity. Instead, code should be readable.
In this case, I would put the False into a config setting with a name that makes it obvious what it is, e.g. fuse_rigid_joints = False (in the config file) and here spec.compiler.fusestatic = myconfig.fuse_rigid_joints
There was a problem hiding this comment.
oh there is a setting for this in the config file already, config.asset.collapse_fixed_joints
| @@ -94,6 +94,9 @@ def _load_model(self, cfg) -> mujoco.MjModel: | |||
| spec = self._load_urdf_spec(asset_path) | |||
| spec.compiler.balanceinertia = True | |||
There was a problem hiding this comment.
similar to the comment below. No idea what this does.
| tau_ff = 4 * [18, 18, 28] | ||
| commands = [3, 1, 3] | ||
|
|
||
| class mjspec_attributes: |
There was a problem hiding this comment.
this should be named in a more readable way.
There was a problem hiding this comment.
Also, should probably live in legged_robot_config.py.
| logger.toc("runtime") | ||
| logger.print_to_terminal() | ||
|
|
||
| # save_to_log_buffers(self.env, self.env.states_to_log) |
| self.save() | ||
| self.save() | ||
|
|
||
| # save_histogram_from_env(self.env, self.env.states_to_log) |
2 participants
Issue ticket number and link
Fixes # (issue)
Describe your changes
Hardcoded njmax and opt.ccd_iterations for MiniCheetah instances because run-time warnings were telling me to increase the values. This is done in mujoco_backend_base.py._load_model().
Instructions for reviewers
The desired behavior is that the runtime warnings disappear and the values njmax and ccd_iterations are actually changed when training MiniCheetah or subclasses
Checklist before requesting a review
ruff format .manually