Adds time-based mdp (observation) functions. #2332
Conversation
…t available for use by mdp functions.
…h_buf so it is visible as an attribute in the documentation.
…f ManagerBasedRLEnv.
TheIndoorDad
requested review from
jsmith-bdai,
Mayankm96 and
kellyguo11
as code owners
| @@ -82,6 +82,9 @@ def __init__(self, cfg: ManagerBasedRLEnvCfg, render_mode: str | None = None, ** | |||
|
|
|||
| # initialize data and constants | |||
| # -- init buffers | |||
| # TODO: this may be redundant since self.episode_length_buf is now initialized in load_managers() to make it | |||
There was a problem hiding this comment.
Instead of moving it to load_managers (which is only for managers and not buffers), maybe it is better to move the buffer initialization further up in the class
There was a problem hiding this comment.
I think I can move it above the super().__init__ call if I change the parameters to be self.cfg.scene.num_envs and self.cfg.sim.device. Seems to be working on my side. I'll make a commit for your review.
| @@ -102,11 +105,20 @@ def max_episode_length(self) -> int: | |||
| """Maximum episode length in environment steps.""" | |||
| return math.ceil(self.max_episode_length_s / self.step_dt) | |||
|
|
|||
| @property | |||
| def curr_episode_length(self) -> torch.Tensor: | |||
There was a problem hiding this comment.
episode_length_buf is a variable you can access outside as it is not a private member. I don't think making a property for it is necessary at this point.
There was a problem hiding this comment.
Sure. I did this purely so the property would show up as an attribute in the documentation, but if you don't think that's necessary (or there's a better way to do it) I will remove.
| @@ -102,11 +105,20 @@ def max_episode_length(self) -> int: | |||
| """Maximum episode length in environment steps.""" | |||
| return math.ceil(self.max_episode_length_s / self.step_dt) | |||
|
|
|||
| @property | |||
| def curr_episode_length(self) -> torch.Tensor: | |||
There was a problem hiding this comment.
Sure. I did this purely so the property would show up as an attribute in the documentation, but if you don't think that's necessary (or there's a better way to do it) I will remove.
| @@ -82,6 +82,9 @@ def __init__(self, cfg: ManagerBasedRLEnvCfg, render_mode: str | None = None, ** | |||
|
|
|||
| # initialize data and constants | |||
| # -- init buffers | |||
| # TODO: this may be redundant since self.episode_length_buf is now initialized in load_managers() to make it | |||
There was a problem hiding this comment.
I think I can move it above the super().__init__ call if I change the parameters to be self.cfg.scene.num_envs and self.cfg.sim.device. Seems to be working on my side. I'll make a commit for your review.
…f directly as suggested. Co-authored-by: Mayank Mittal <[email protected]> Signed-off-by: TheIndoorDad <[email protected]>
…re `super().__init__` call.
Description
As discussed in #2284, I was writing an observation function to pass the time remaining in an episode (in seconds) to an observation term in a Manager-based environment, and found this could not be done without modifying
ManagerBasedRLEnvto initializeepisode_length_bufbefore managers are loaded. Here is a summary of changes made:episode_length_bufin :meth:load_managers()of :class:~isaaclab.envs.ManagerBasedRLEnvto make it available for use in mdp functions. Note: existing initialization in :meth:__init__left in place in case it is needed for other use cases. Potentially redundant? Assess.~isaaclab.envs.ManagerBasedRLEnv.curr_episode_lengthto :class:~isaaclab.envs.ManagerBasedRLEnvwhich returns reshapedepisode_length_bufso it is visible as an attribute in the documentation.~isaaclab.envs.mdp.observationsmodule, :func:~isaaclab.envs.mdp.observations.current_time_sand :func:~isaaclab.envs.mdp.observations.remaining_time_s.I'm not certain whether the documentation will be updated automatically or if there are further steps I need to take. When I build the documentation on my machine it is updated, but the outputs are ignored by git. Please let me know if there's anything else I need to do.
I could also use some advice on tests (apologies in advance for my lack of experience here, my background is not in software dev). Locally I modified the
Isaac-Velocity-Rough-Anymal-C-v0task to add the two new observation functions, and began to train a policy in rsl_rl using the providedscripts/reinforcement_learning/rsl_rl/train.pyscript, and both were available to be viewed and appeared to be working correctly. I tried to run the existing suite of unit tests but it gave me an error I don't understand (see below). I also started to create a new script similar toisaaclab/test/envs/check_manager_based_env_anymal_locomotion.pybut that would have required a policy trained using the new observation functions (which I can produce, but wasn't sure if that would be worthwhile since it wouldn't be available to others).Output when running
./isaaclab.sh -t:Cheers.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there