Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a pre/post processing concept for action terms, refactors concrete ActionTerm implementations into a new actions.py module, and introduces an environment-level postprocessing hook intended to run after stepping.
Changes:
- Introduced
mode: Literal["pre","post"]onActionTermCfgand added mode-based filtering/dispatch inActionManager. - Moved concrete action terms (e.g.,
DeltaQposTerm,EefPoseTerm) intoembodichain.lab.gym.envs.managers.actions. - Added a
_postprocess_action()hook toBaseEnv.step()and started wiring an override inEmbodiedEnv; expanded unit tests for mode behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gym/envs/managers/test_action_manager.py | Adds tests for mode dispatch and the new clamp term. |
| embodichain/lab/gym/utils/gym_utils.py | Updates default module list used for resolving action term callables from config. |
| embodichain/lab/gym/envs/managers/cfg.py | Adds mode field to ActionTermCfg. |
| embodichain/lab/gym/envs/managers/actions.py | New module containing concrete ActionTerm implementations, plus ActionClampTerm. |
| embodichain/lab/gym/envs/managers/action_manager.py | Adds mode-aware term selection, reporting, and dimension helpers. |
| embodichain/lab/gym/envs/managers/init.py | Re-exports action manager and action terms via star imports. |
| embodichain/lab/gym/envs/embodied_env.py | Introduces (currently stubbed) _postprocess_action() override. |
| embodichain/lab/gym/envs/base_env.py | Adds _postprocess_action() hook and invokes it in step(). |
| AGENTS.md | Removes a naming warning line in the agent documentation. |
Comments suppressed due to low confidence (1)
embodichain/lab/gym/envs/managers/action_manager.py:151
- With pre/post modes,
action_typestill returnsself._term_names[0](first term in config order), which can be a "post" term. For backward compatibility, this should likely resolve to the active pre term (e.g.,self._mode_term_names["pre"][0]) or otherwise be mode-aware to avoid reporting the wrong policy action type.
def action_type(self) -> str:
"""The active action type (term name) for backward compatibility."""
return self._term_names[0]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+345
to
+353
| mode: Literal["pre", "post"] = "pre" | ||
| """The mode for the action term. | ||
|
|
||
| - ``pre``: Preprocess raw action from policy (default). This is applied before | ||
| the action is sent to the robot control. | ||
| - ``post``: Postprocess the action after it has been processed by another term. | ||
| This is useful for applying additional transformations like noise, clipping, | ||
| or filtering to the output actions. | ||
| """ |
Comment on lines
152
to
+167
| @property | ||
| def total_action_dim(self) -> int: | ||
| """Total dimension of actions (sum of all term dimensions).""" | ||
| return sum(t.action_dim for t in self._terms.values()) | ||
|
|
||
| def process_action(self, action: EnvAction) -> EnvAction: | ||
| def get_action_dim_by_mode(self, mode: Literal["pre", "post"]) -> int: | ||
| """Get total action dimension for terms of a specific mode. | ||
|
|
||
| Args: | ||
| mode: The mode to filter by ("pre" or "post"). | ||
|
|
||
| Returns: | ||
| Sum of action dimensions for terms with the specified mode. | ||
| """ | ||
| mode_terms = self.get_functors_by_mode(mode) | ||
| return sum(term.action_dim for _, term in mode_terms) |
yangchen73
reviewed
Mar 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds support for two processing modes (pre and post) in ActionTermCfg and ActionManager, and refactors action terms into a separate module.
Changes Made:
- Added mode: Literal["pre", "post"] = "pre" to support pre-processing
and post-processing modes
- pre (default): Preprocess raw action from policy before sending to
robot control
- post: Postprocess the action after it has been processed by another
term (useful for clamping, filtering, etc.)
- Moved all concrete ActionTerm implementations (DeltaQposTerm,
QposTerm, QposNormalizedTerm, EefPoseTerm, QvelTerm, QfTerm,
ActionClampTerm) to a new actions.py module
- Added comprehensive docstrings with examples to each action term
- ActionManager now imports action terms from actions.py for backward
compatibility
- Modified process_action() to accept a mode parameter to switch between
pre/post processing
- Added get_functors_by_mode() method to get terms filtered by mode
- Added get_action_dim_by_mode() method to get action dimension for
specific mode
- Updated str() to display mode information in the table
values to specified limits
- Added 11 new tests covering mode functionality
Type of change
Checklist
black .command to format the code base.