feat: Introduce G1 Energy Benchmark Environment (Fluidity Index)#1
Open
ngoiyaeric wants to merge 6 commits into
Open
feat: Introduce G1 Energy Benchmark Environment (Fluidity Index)#1ngoiyaeric wants to merge 6 commits into
ngoiyaeric wants to merge 6 commits into
Conversation
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Review Summary by QodoAdd G1 Energy Benchmark environment with battery and token mechanics
WalkthroughsDescription• Introduces G1 Energy Benchmark environment with battery and token tracking • Implements custom energy drain based on robot torque consumption • Adds token earning mechanism tied to velocity tracking quality • Includes charging station logic at world origin with token cost • Provides custom observations and rewards for energy-aware RL training Diagramflowchart LR
A["G1FlatEnvCfg"] -->|extends| B["G1EnergyEnvCfg"]
C["ManagerBasedRLEnv"] -->|extends| D["G1EnergyEnv"]
B -->|configures| D
D -->|tracks| E["Battery State"]
D -->|tracks| F["Token State"]
E -->|drains via| G["Torque-based Energy"]
F -->|earned via| H["Velocity Tracking Quality"]
E -->|recharged at| I["Charging Station"]
I -->|costs| F
D -->|provides| J["Custom Observations"]
D -->|provides| K["Custom Rewards"]
D -->|provides| L["Termination Conditions"]
File Changes1. source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/__init__.py
|
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughAdds a new G1 energy-aware locomotion task: env class, env config, MDP utilities (observations, rewards, terminations), package initializers, and Gymnasium registration exposing "Isaac-Velocity-Energy-G1-v0". Changes
Sequence DiagramsequenceDiagram
participant Agent as RL Agent
participant Env as G1EnergyEnv
participant Physics as Physics Engine
participant Energy as Energy System
participant Charging as Charging Station
participant Managers as Reward/Termination Managers
Agent->>Env: step(action)
Env->>Env: apply action & record pre-step data
loop decimation steps
Env->>Physics: simulate physics step
Physics-->>Env: sensor/torque data
end
Env->>Energy: compute energy drain from torques
Energy-->>Env: update battery_buf (clamped)
Env->>Energy: compute token earnings from tracking error
Energy-->>Env: update tokens_buf
Env->>Charging: check proximity & token availability
alt within radius AND tokens >= cost
Charging->>Energy: refill battery_buf (deduct tokens)
Charging->>Env: emit at_charging_station event
end
Env->>Managers: compute reward & termination
Managers-->>Env: reward, done flags
alt reset required
Env->>Env: _reset_idx -> reset battery_buf, tokens_buf, env state
end
Env->>Agent: return (obs, reward, terminated, truncated, extras)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/__init__.py (1)
6-8: Consider reordering imports per PEP 8.Standard library imports (
gymnasium) should come before local imports. This is a minor style nit.♻️ Suggested import order
+import gymnasium as gym + from .env import G1EnergyEnv from .env_cfg import G1EnergyEnvCfg -import gymnasium as gym🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/__init__.py` around lines 6 - 8, Reorder the imports in this module to follow PEP 8 by placing the third-party/standard import before the local package imports: import gymnasium as gym should come before the relative imports of G1EnergyEnv and G1EnergyEnvCfg (the symbols to look for are G1EnergyEnv, G1EnergyEnvCfg, and gym).source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.py (1)
17-20: Consider normalizingtoken_countfor training stability.Unlike
battery_level,token_countreturns unbounded raw values. This could lead to large variance in observations as tokens accumulate over time, potentially affecting training stability. Consider normalizing by a maximum expected token value or applying a transform (e.g.,tanh).♻️ Example normalization approach
+MAX_EXPECTED_TOKENS = 10.0 # Tune based on expected accumulation + def token_count(env: ManagerBasedRLEnv) -> torch.Tensor: """Returns the current token count of the robot.""" # (num_envs, 1) - return env.tokens_buf.unsqueeze(-1) + # Normalize using tanh for bounded output + return torch.tanh(env.tokens_buf / MAX_EXPECTED_TOKENS).unsqueeze(-1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.py` around lines 17 - 20, The token_count observation currently returns raw, unbounded values from env.tokens_buf via token_count(env: ManagerBasedRLEnv); normalize it to improve training stability by transforming the tensor before returning—e.g., divide by a configurable max_expected_tokens (or apply torch.tanh) and keep the (num_envs, 1) shape; ensure the normalization uses a clear parameter name (e.g., max_expected_tokens or token_scale) accessible where observations are constructed and document that token_count returns the normalized tensor so it remains consistent with battery_level scaling.source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py (1)
17-25: Remove redundant_cfgassignment.The parent class
ManagerBasedRLEnv.__init__already stores the config asself.cfg. Theself._cfg = cfgassignment is redundant and could cause confusion about which attribute to use.♻️ Proposed fix
def __init__(self, cfg, **kwargs): # 1. Allocate custom buffers before calling super().__init__() # These will be initialized to 1.0 (full battery) and 0.0 (no tokens) self.battery_buf = None self.tokens_buf = None - self._cfg = cfg # Super init will call load_managers, so we will initialize the buffers inside load_managers super().__init__(cfg, **kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py` around lines 17 - 25, In the __init__ of the environment class remove the redundant self._cfg = cfg assignment (the parent ManagerBasedRLEnv.__init__ already stores the config as self.cfg) and rely on self.cfg throughout; keep the custom buffer attributes (battery_buf and tokens_buf) initialization and the super().__init__(cfg, **kwargs) call, and update any references to use self.cfg instead of self._cfg if present.source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.py (1)
18-24: Avoid creating tensors inside the reward function hot path.Creating
torch.tensor()objects on every step incurs overhead from device allocation. Use tensor multiplication ortorch.wherewith fill values directly.♻️ Proposed fix using scalar multiplication
def empty_battery_penalty(env: ManagerBasedRLEnv) -> torch.Tensor: """Heavily penalize when battery hits exactly 0.""" - return torch.where( - env.battery_buf <= 0.01, - torch.tensor(-10.0, device=env.device), - torch.tensor(0.0, device=env.device), - ) + return (env.battery_buf <= 0.01).float() * -10.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.py` around lines 18 - 24, The reward function empty_battery_penalty currently allocates new tensors each step via torch.tensor(..., device=env.device); replace those allocations by using tensor operations on existing buffers — e.g. build the mask = env.battery_buf <= 0.01 and return mask.to(env.battery_buf.dtype) * -10.0 or use torch.where(mask, torch.full_like(env.battery_buf, -10.0), torch.zeros_like(env.battery_buf)) so no per-step torch.tensor() device allocations occur; update empty_battery_penalty to use env.battery_buf, mask, and full_like/zeros_like or scalar-multiplication to keep tensors on the correct device and dtype.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/__init__.py`:
- Around line 8-10: Update the stale comment in __init__.py to reflect that this
package now re-exports symbols from g1_energy; replace the lines that claim the
file is empty and doesn't expose configs with a short comment indicating that
g1_energy is intentionally re-exported (e.g., "Re-export g1_energy configs for
parent-package imports") and keep the existing from .g1_energy import *
statement unchanged so imports continue to work.
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.py`:
- Line 16: The import in env_cfg.py uses the repository prefix "source." which
breaks installed package imports; change the import of custom_mdp to the
package-relative path (drop the leading "source."), e.g., import
source.isaaclab_tasks.isaaclab_tasks... -> import
isaaclab_tasks.manager_based.locomotion.velocity.config.g1_energy.mdp as
custom_mdp (locate the import statement referencing custom_mdp in env_cfg.py and
update it to the correct package path).
- Line 43: The DoneTerm for battery depletion is incorrectly marked as a
timeout; update the termination created at self.terminations.battery_empty
(DoneTerm(func=custom_mdp.battery_empty, ...)) so it is treated as a failure
termination rather than a time_out—i.e., remove or set time_out to False so
battery_empty contributes to reset_terminated (not reset_time_outs).
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py`:
- Around line 141-153: The current _reset_idx implementation logs avg_battery
and avg_tokens after the buffers are reset, so episode-end stats are lost; to
fix, sample the battery_buf and tokens_buf values for the provided env_ids
before you call super()._reset_idx (or before you overwrite them), compute their
means, and store those pre-reset values into
self.extras["log"]["Metrics/avg_battery"] and
self.extras["log"]["Metrics/avg_tokens"] instead of the post-reset values; keep
the subsequent buffer reset (self.battery_buf[env_ids] = self.max_battery,
self.tokens_buf[env_ids] = 0.0) and ensure this change is made inside the
_reset_idx method.
- Around line 114-118: The current reset index extraction uses
self.reset_buf.nonzero(as_tuple=False).squeeze(-1), which can yield a 0-D tensor
when there is a single environment and break the downstream _reset_idx call;
replace the squeeze(-1) with .flatten() to guarantee a 1-D tensor of indices
(e.g., assign reset_env_ids = self.reset_buf.nonzero(as_tuple=False).flatten()),
then keep the existing check and calls to
self.recorder_manager.record_pre_reset(reset_env_ids) and
self._reset_idx(reset_env_ids).
---
Nitpick comments:
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/__init__.py`:
- Around line 6-8: Reorder the imports in this module to follow PEP 8 by placing
the third-party/standard import before the local package imports: import
gymnasium as gym should come before the relative imports of G1EnergyEnv and
G1EnergyEnvCfg (the symbols to look for are G1EnergyEnv, G1EnergyEnvCfg, and
gym).
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py`:
- Around line 17-25: In the __init__ of the environment class remove the
redundant self._cfg = cfg assignment (the parent ManagerBasedRLEnv.__init__
already stores the config as self.cfg) and rely on self.cfg throughout; keep the
custom buffer attributes (battery_buf and tokens_buf) initialization and the
super().__init__(cfg, **kwargs) call, and update any references to use self.cfg
instead of self._cfg if present.
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.py`:
- Around line 17-20: The token_count observation currently returns raw,
unbounded values from env.tokens_buf via token_count(env: ManagerBasedRLEnv);
normalize it to improve training stability by transforming the tensor before
returning—e.g., divide by a configurable max_expected_tokens (or apply
torch.tanh) and keep the (num_envs, 1) shape; ensure the normalization uses a
clear parameter name (e.g., max_expected_tokens or token_scale) accessible where
observations are constructed and document that token_count returns the
normalized tensor so it remains consistent with battery_level scaling.
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.py`:
- Around line 18-24: The reward function empty_battery_penalty currently
allocates new tensors each step via torch.tensor(..., device=env.device);
replace those allocations by using tensor operations on existing buffers — e.g.
build the mask = env.battery_buf <= 0.01 and return
mask.to(env.battery_buf.dtype) * -10.0 or use torch.where(mask,
torch.full_like(env.battery_buf, -10.0), torch.zeros_like(env.battery_buf)) so
no per-step torch.tensor() device allocations occur; update
empty_battery_penalty to use env.battery_buf, mask, and full_like/zeros_like or
scalar-multiplication to keep tensors on the correct device and dtype.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/__init__.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/__init__.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/__init__.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/terminations.py
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Note Docstrings generation - SUCCESS |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 1, 2026
Docstrings generation was requested by @ngoiyaeric. * #1 (comment) The following files were modified: * `source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py` * `source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.py` * `source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.py` * `source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.py` * `source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/terminations.py`
This was referenced Mar 1, 2026
|
✅ Created PR with unit tests: #3 |
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py (1)
78-80: Consider making velocity error scaling configurable.The hardcoded
0.5divisor intorch.exp(-vel_error / 0.5)controls how strictly velocity tracking affects token earning. This tuning parameter could be exposed inG1EnergyEnvCfgfor easier experimentation.♻️ Proposed refactor
In
env_cfg.py:charge_token_cost: float = 1.0 charging_station_radius: float = 1.0 +vel_error_scale: float = 0.5 # Sensitivity of token earning to velocity tracking errorIn
env.py:-job_quality = torch.exp(-vel_error / 0.5) +job_quality = torch.exp(-vel_error / self.cfg.vel_error_scale)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py` around lines 78 - 80, The hardcoded velocity-error scaling (0.5) inside job_quality = torch.exp(-vel_error / 0.5) should be made configurable via G1EnergyEnvCfg so tuning is easier; add a new field (e.g., vel_error_scale or vel_tracking_scale) to G1EnergyEnvCfg with a sensible default, update any env config parsing, and replace the literal 0.5 in env.py with self.cfg.vel_error_scale (or the chosen name) when computing job_quality and tokens_earned so the behavior is driven by the config rather than a magic number.source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.py (1)
57-58: Consider replacing stub lambda with a named no-op function.The lambda with unused arguments is a placeholder for the
at_charging_stationevent. While functional, a named function improves clarity and avoids the Ruff ARG005 warnings.♻️ Proposed refactor
Add a no-op function at module level or in the mdp package:
def _noop_event(env, env_ids): """No-op event handler for charging station proximity.""" passThen use it:
- self.events.at_charging_station = EventTerm(func=lambda env, env_ids: None, mode="at_charging_station") + self.events.at_charging_station = EventTerm(func=_noop_event, mode="at_charging_station")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.py` around lines 57 - 58, Replace the inline lambda used for self.events.at_charging_station with a named no-op function to improve clarity and silence Ruff ARG005; add a module-level function (e.g., def _noop_event(env, env_ids): pass) or import one from the mdp package, then instantiate EventTerm(func=_noop_event, mode="at_charging_station") instead of the lambda so the EventTerm call and symbol self.events.at_charging_station remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.py`:
- Around line 57-58: Replace the inline lambda used for
self.events.at_charging_station with a named no-op function to improve clarity
and silence Ruff ARG005; add a module-level function (e.g., def _noop_event(env,
env_ids): pass) or import one from the mdp package, then instantiate
EventTerm(func=_noop_event, mode="at_charging_station") instead of the lambda so
the EventTerm call and symbol self.events.at_charging_station remain unchanged.
In
`@source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.py`:
- Around line 78-80: The hardcoded velocity-error scaling (0.5) inside
job_quality = torch.exp(-vel_error / 0.5) should be made configurable via
G1EnergyEnvCfg so tuning is easier; add a new field (e.g., vel_error_scale or
vel_tracking_scale) to G1EnergyEnvCfg with a sensible default, update any env
config parsing, and replace the literal 0.5 in env.py with
self.cfg.vel_error_scale (or the chosen name) when computing job_quality and
tokens_earned so the behavior is driven by the config rather than a magic
number.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/__init__.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/__init__.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/env_cfg.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/observations.pysource/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1_energy/mdp/rewards.py
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
ngoiyaeric
changed the title
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
Collaborator
Author
FIX.mp4 |
Acknowledged. |
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
Collaborator
Author
grok-video-81b989a1-c83e-4d20-9193-b0dc645c90c6.mp4 |
Acknowledged. |
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
Collaborator
Author
|
Synchronize the main branch without merge issues |
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
1 participant
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.
This PR introduces a new energy benchmark environment for the Unitree G1 robot (
Isaac-Velocity-Energy-G1-v0). The environment tracks battery and token states directly in a custom subclass ofManagerBasedRLEnvby overridingstep,load_managers, and_reset_idx. It utilizes a custom token earning mechanism and charging logic at the world origin, and feeds these states back to the policy via custom observations. This acts as a foundation for energy-efficient humanoid RL research.PR created automatically by Jules for task 3027240647789121151 started by @ngoiyaeric
Summary by CodeRabbit