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

Add prev_prev_action to ActionTerm #1614

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fan-ziqi
Copy link
Contributor

Description

This PR introduces a new feature in the Antion Manager by adding a prev_prev_action to ActionTerm. The purpose of this addition is to allow the computation of second-order smoothing for actions, providing a smoother transition between actions over time.

Such as

def smoothness_2(env: ManagerBasedRLEnv) -> torch.Tensor:
    # Penalize changes in actions
    diff = torch.square(env.action_manager.action - 2 * env.action_manager.prev_action + env.action_manager.prev_prev_action)
    diff = diff * (env.action_manager.prev_action[:, :] != 0)  # ignore first step
    diff = diff * (env.action_manager.prev_prev_action[:, :] != 0)  # ignore second step
    return torch.sum(diff, dim=1)

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@Mayankm96
Copy link
Contributor

Mayankm96 commented Jan 5, 2025

To prevent a slippery slope of prev_prev_prev_prev_..._action, I suggest we instead make a tensor for action history (N, H, N_a), where the action manager receives something like a history length in its configuration (similar to what we do for a contact sensor).

@fan-ziqi
Copy link
Contributor Author

fan-ziqi commented Jan 7, 2025

Where should I add history_length in ActionsCfg?

@configclass
class ActionsCfg:
    """Action specifications for the MDP."""

    joint_pos = mdp.JointPositionActionCfg(asset_name="robot", joint_names=[".*"], scale=0.5, use_default_offset=True)

@kellyguo11
Copy link
Contributor

Where should I add history_length in ActionsCfg?

@configclass
class ActionsCfg:
    """Action specifications for the MDP."""

    joint_pos = mdp.JointPositionActionCfg(asset_name="robot", joint_names=[".*"], scale=0.5, use_default_offset=True)

I think we can add a history_length attribute to the ActionTermCfg - https://github.com/isaac-sim/IsaacLab/blob/main/source/extensions/omni.isaac.lab/omni/isaac/lab/managers/manager_term_cfg.py#L77. then in the example above, the predefined action terms such as JointPositionActionCfg can all leverage it.

@fan-ziqi
Copy link
Contributor Author

In ActionTerm, we don't have ActionGroup like ObservationGroup in ObservationTerm, so if I set history_length for every action terms separately, how should I store history for different action terms?

(If I add a history_length attribute to the ActionTermCfg, I can't get self.cfg.history_length such as self._action_history = torch.zeros(self.num_envs, self.cfg.history_length, self.total_action_dim, device=self.device). )

@kellyguo11
Copy link
Contributor

Ah I see what you mean, it is tricky indeed. I guess one way is to just use the max history length of all of the terms and populate the buffer for them all in the Action Manager, similar to how the current prev action is done. Or maybe we can use a special named term in the action manager cfg for specifying the history length? both seems a bit messy though.

@Mayankm96 what would you recommend? maybe I'm missing something in my understanding of the action manager.

@kellyguo11
Copy link
Contributor

@jtigue-bdai

@jtigue-bdai
Copy link
Collaborator

jtigue-bdai commented Jan 23, 2025

As @fan-ziqi eluded to we don't have a dedicated ActionManagerCfg that contains a list of bespoke cfg parametere like ObservationGroupCfg does. We could do something like:

Create a base ActionManagerCfg() that users could inherit from that contains history_length. The ActionManager will have to be changed to look for that history_length parameter as well as individual ActionTermCfg.history_length to decide how to setup history buffers. (Much like how the ObservationManager does with the ObservationGroupCfg.history_length and ObservationTermCfg.history_length

IsaacLab source code would add to omni.isaac.lab/omni/isaac/lab/managers/manager_term_cfg.py:

@configclass 
class ActionManagerCfg:
    history_length: int = 0
    """Number of past actions to store in the zero-initialized action buffers.
    Defaults to 0, which means that only the current data is stored (no history).
    If flatten_history_dim is set to True, the source data of shape (N, H, D, ...) where N is the batch dimension and
    H is the history length will be reshaped to a 2D tensor of shape (N, H*D*...). Otherwise, the data will be returned as is.
    """
    flatten_history_dim: bool = True
    """Whether or not the observation manager should flatten history-based observation terms to a 2D (N, D) tensor. Defaults to True."""

Users application code would add something like:

from omni.isaac.lab.managers import ActionManagerCfg

@configclass
class MyActionsCfg(ActionManagerCfg):
    action_1 = ActionTermCfg(.....)
    action_2 = JointActionCfg(.....)

This however is a slight deviation to our current configuration management scheme. Our managers don't have a bespoke configclass object they are looking for (like how the Terms do).

@jtigue-bdai jtigue-bdai requested review from jtigue-bdai and removed request for Dhoeller19 January 23, 2025 16:36
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.

4 participants