Move newton-actuators into Newton repository#2297
Closed
eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
Closed
Move newton-actuators into Newton repository#2297eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
eric-heiden wants to merge 4 commits intonewton-physics:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughIntegrates a full actuator subsystem into the repository: new actuator implementations, Warp kernels, USD parsing, public exports, docs, tests, and removes the external Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as ModelBuilder
participant Model as Model / Simulation
participant Act as Actuator
participant State as State
participant Control as Control
participant Kernels as Warp/Torch Kernels
Builder->>Model: add_actuator(ParsedActuator / class+kwargs)
Model->>Act: instantiate with indices + params
loop per timestep
Model->>State: read current joint_q/joint_qd
Model->>Control: read targets / joint_act
Model->>Act: step(sim_state, sim_control, current_state, next_state, dt)
Act->>Kernels: run controller kernel or Torch network
Kernels-->>Act: torques/forces
Act->>Control: write forces into control.joint_f
Act->>State: update actuator state into next_state (if stateful)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
newton/actuators.py (1)
13-24: Avoid freezing the USD parser helpers into the public API before that decision is settled.Exporting
ParsedActuatorandparse_actuator_primfromnewton.actuatorsmakes them public immediately. If we are not ready to support them long-term, keep them internal for now so we do not need a deprecation cycle just to undo this surface later.As per coding guidelines, "Breaking changes require a deprecation first. Do not remove or rename public API symbols without deprecating them in a prior release."
Also applies to: 26-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/actuators.py` around lines 13 - 24, The file currently re-exports USD parser helpers (ParsedActuator, parse_actuator_prim) from ._src.actuators, which makes them public; remove them from the public import list so they remain internal. Edit the import statement that lists Actuator, ActuatorDCMotor, ActuatorDelayedPD, ActuatorNetLSTM, ActuatorNetMLP, ActuatorPD, ActuatorPID, ActuatorRemotizedPD, ParsedActuator, parse_actuator_prim to drop ParsedActuator and parse_actuator_prim (or import them under private names like _ParsedActuator/_parse_actuator_prim if internal use is needed), and update any module __all__ or export lists accordingly so only the intended Actuator* classes are part of the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/actuators.rst`:
- Around line 87-128: The page mixes two different Actuator.step() call styles;
unify to the full public signature used by stateful actuators (e.g.,
Actuator.step(sim_state, sim_control, current_state, next_state, dt)) and update
both examples to use that exact ordering: change the simple loop to call
actuator.step(sim_state, sim_control, None, None, dt) and keep the PID example
calling pid_actuator.step(sim_state, sim_control, current_state, next_state,
dt=0.01); ensure references to ActuatorPID.is_stateful() and ActuatorPID.state()
remain to show when to create and supply current_state/next_state.
- Around line 104-116: The example uses wp.array(...) but never imports Warp;
add the missing import (e.g., "import warp as wp") at the top of the example
block before any wp.array calls so the ActuatorPID example (symbols:
ActuatorPID, wp.array, pid_actuator) can run; ensure the import appears
alongside the other imports in that snippet.
- Around line 147-149: The docs currently reference the private helper
`newton.usd.parse_usd`; update the sentence to reference the public entry point
instead by replacing the Sphinx link `:func:\`newton.usd.parse_usd\`` with
`:meth:\`~newton.ModelBuilder.add_usd\`` (or use plain ``parse_usd()`` if you
prefer a non-linked mention), and ensure the text clarifies that actuator prims
are parsed via the public `ModelBuilder.add_usd` path rather than the private
`parse_usd` implementation.
In `@newton/_src/actuators/actuators/dc_motor.py`:
- Around line 97-117: The constructor currently only checks array lengths (in
the loop iterating over
"kp","kd","max_force","saturation_effort","velocity_limit") but does not
validate values; add a guard in the DC motor constructor to reject non-positive
entries in velocity_limit by iterating over velocity_limit and raising a
ValueError (with a clear message including the offending value/index and
expected >0) if any entry <= 0; reference the velocity_limit validation in the
constructor (and mention pd_controller_kernel uses velocity_limit for
torque-speed division) so callers can't pass zeros or negatives that later cause
inf/NaN or incorrect saturation behavior.
In `@newton/_src/actuators/actuators/delayed_pd.py`:
- Around line 78-126: The constructor (__init__) must validate the delay
parameter and reject non-positive values to avoid downstream buffer allocation
and modulo-by-zero errors in state() and the actuator stepping logic; add a
check near the top of __init__ (before any buffer/state setup or assignment to
self.delay) that raises ValueError if delay < 1 (e.g., "raise ValueError('delay
must be >= 1')"), ensuring callers cannot construct DelayedPD actuators with
delay <= 0 which later cause modulo-by-zero in methods that index using
self.delay.
In `@newton/_src/actuators/actuators/net_mlp.py`:
- Around line 42-85: SCALAR_PARAMS must include all per-actuator scalar settings
so builder batching won't merge incompatible configs: add "pos_scale",
"vel_scale", "torque_scale", "input_order", and "input_idx" to SCALAR_PARAMS; in
resolve_arguments (ActuatorNetMLP.resolve_arguments) normalize input_idx to a
hashable tuple (e.g., if input_idx is not None convert list -> tuple) and return
it, and ensure any other resolve_arguments block for this class (the similar
block referenced at 87-103) applies the same tuple normalization and uses the
updated SCALAR_PARAMS.
In `@newton/_src/actuators/actuators/pd.py`:
- Around line 29-61: resolve_arguments currently drops the gear/transmission
parameter and the ActuatorPD constructor does not accept or store it, breaking
code that expects act.gear to exist; update resolve_arguments (function
resolve_arguments) to include a "gear" (or "transmission") entry with a sensible
default (e.g., 1.0) and add a corresponding parameter to ActuatorPD.__init__
(e.g., gear: wp.array[float] | float | None = None), store it on the instance
(self.gear or self.transmission), and ensure downstream uses of the actuator
output (where torque/force is computed) multiply/divide by this gear value so
act.gear is exposed and used consistently.
- Around line 114-137: The pd_controller_kernel assumes scalar indices
(state_idx = state_indices[i]) but ActuatorPD may be given 2D index arrays for
multi-input actuators; fix by either (A) validating in ActuatorPD.__init__ that
input_indices/output_indices are 1D and raising a clear ValueError if not (check
shapes of self.input_indices/self.output_indices), or (B) refactor
pd_controller_kernel to support 2D indices by nested-iterating over the second
dimension (e.g., for j in range(num_inputs_per_actuator): state_idx =
state_indices[i, j]; accumulate contributions from current_pos[state_idx],
current_vel[state_idx], and corresponding control inputs) and summing those
contributions into the actuator output; update any related kernel
inputs/metadata accordingly (references: ActuatorPD.__init__,
pd_controller_kernel, state_indices, state_idx, current_pos, current_vel,
input_indices, output_indices).
In `@newton/_src/actuators/actuators/remotized_pd.py`:
- Around line 122-135: In the RemotizedPD constructor in remotized_pd.py, after
converting lookup_angles to a wp.array (before assigning self.lookup_angles),
validate that the table is strictly increasing by checking consecutive
differences are all > 0 (e.g., compute diffs of lookup_angles and verify no diff
<= 0); if any non-positive difference is found, raise a ValueError stating that
lookup_angles must be strictly increasing (include lengths/indices for clarity).
Ensure this check runs on the wp.array form you assign to self.lookup_angles so
unsorted or repeated angles are rejected early.
In `@newton/_src/actuators/kernels.py`:
- Around line 83-90: Add a runtime validation to ensure all entries of
velocity_limit are > 0 when an ActuatorDCMotor is constructed; update
ActuatorDCMotor.resolve_arguments (or its __init__) to check the velocity_limit
array passed in and raise a clear ValueError if any value is <= 0 so the kernel
lines using velocity_limit (where max_torque/min_torque compute sat * (1.0 - vel
/ vel_lim)) cannot divide by zero; reference the ActuatorDCMotor class and its
resolve_arguments method to locate where to perform this check and include the
failing value(s) in the error message.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 18-19: The parsed transmission field (transmission on the actuator
data object returned by parse_actuator_prim) is being dropped because
import_usd's call to builder.add_actuator(...) only forwards parsed.kwargs;
modify the importer to include parsed.transmission when constructing the
actuator (e.g., fold it into parsed.kwargs under a clear key like "transmission"
before calling builder.add_actuator, or change the add_actuator call to accept a
transmission=parsed.transmission parameter) so that the transmission data
produced by parse_actuator_prim is actually consumed; update any related call
sites and type hints for add_actuator/kwargs to reflect this new parameter
usage.
In `@newton/_src/utils/import_usd.py`:
- Around line 2761-2768: The importer loop is dropping the parsed actuator's
transmission vector; update the code in the loop that calls parse_actuator_prim
and builder.add_actuator so that any transmission returned by
parse_actuator_prim (e.g. parsed.transmission) is threaded into add_actuator —
either by adding transmission=parsed.transmission to the call or by inserting it
into parsed.kwargs before calling builder.add_actuator; if a transmission is
present but invalid, explicitly reject or raise a clear error instead of
silently ignoring it.
In `@newton/tests/test_actuators.py`:
- Around line 56-65: Tests pass a kwarg named max_force to ActuatorPD, but
ActuatorPD.__init__ accepts constant_force (no max_force), causing construction
to fail; update the tests (e.g., test_pd_actuator_creation and the similar test
around the next case) to replace max_force=... with constant_force=... (use the
same wp.array values) so the constructor matches ActuatorPD(input_indices,
output_indices, kp, kd, constant_force=...) or alternatively add a max_force
parameter to ActuatorPD that maps to the existing constant_force internally, but
prefer updating the tests for minimal change.
---
Nitpick comments:
In `@newton/actuators.py`:
- Around line 13-24: The file currently re-exports USD parser helpers
(ParsedActuator, parse_actuator_prim) from ._src.actuators, which makes them
public; remove them from the public import list so they remain internal. Edit
the import statement that lists Actuator, ActuatorDCMotor, ActuatorDelayedPD,
ActuatorNetLSTM, ActuatorNetMLP, ActuatorPD, ActuatorPID, ActuatorRemotizedPD,
ParsedActuator, parse_actuator_prim to drop ParsedActuator and
parse_actuator_prim (or import them under private names like
_ParsedActuator/_parse_actuator_prim if internal use is needed), and update any
module __all__ or export lists accordingly so only the intended Actuator*
classes are part of the public API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d006fe4c-35e4-40b0-a0d2-589fd7465231
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
docs/concepts/actuators.rstdocs/generate_api.pydocs/index.rstnewton/__init__.pynewton/_src/actuators/__init__.pynewton/_src/actuators/actuators/__init__.pynewton/_src/actuators/actuators/base.pynewton/_src/actuators/actuators/dc_motor.pynewton/_src/actuators/actuators/delayed_pd.pynewton/_src/actuators/actuators/net_lstm.pynewton/_src/actuators/actuators/net_mlp.pynewton/_src/actuators/actuators/pd.pynewton/_src/actuators/actuators/pid.pynewton/_src/actuators/actuators/remotized_pd.pynewton/_src/actuators/kernels.pynewton/_src/actuators/usd_parser.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_usd.pynewton/_src/utils/selection.pynewton/actuators.pynewton/tests/test_actuators.pynewton/tests/test_actuators_integration.pypyproject.toml
💤 Files with no reviewable changes (1)
- pyproject.toml
Comment on lines
+87
to
+128
| After finalizing the model, iterate over ``model.actuators`` each timestep:: | ||
|
|
||
| state = model.state() | ||
| control = model.control() | ||
|
|
||
| # Set targets on control | ||
| # ... | ||
|
|
||
| for actuator in model.actuators: | ||
| actuator.step(state, control) | ||
|
|
||
| Stateful Actuators | ||
| ^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Stateful actuators (PID, delayed, neural-network) require double-buffered | ||
| state objects:: | ||
|
|
||
| from newton.actuators import ActuatorPID | ||
|
|
||
| indices = wp.array([0, 1], dtype=wp.uint32) | ||
| pid_actuator = ActuatorPID( | ||
| input_indices=indices, | ||
| output_indices=indices, | ||
| kp=wp.array([100.0, 100.0], dtype=wp.float32), | ||
| ki=wp.array([10.0, 10.0], dtype=wp.float32), | ||
| kd=wp.array([5.0, 5.0], dtype=wp.float32), | ||
| max_force=wp.array([50.0, 50.0], dtype=wp.float32), | ||
| integral_max=wp.array([10.0, 10.0], dtype=wp.float32), | ||
| constant_force=wp.array([0.0, 0.0], dtype=wp.float32), | ||
| ) | ||
|
|
||
| # Check if actuator needs state management | ||
| if pid_actuator.is_stateful(): | ||
| # Create double-buffered states | ||
| state_a = pid_actuator.state() | ||
| state_b = pid_actuator.state() | ||
|
|
||
| # Simulation loop with state swapping | ||
| current_state, next_state = state_a, state_b | ||
| for step in range(num_steps): | ||
| pid_actuator.step(sim_state, sim_control, current_state, next_state, dt=0.01) | ||
| current_state, next_state = next_state, current_state # Swap buffers |
Contributor
There was a problem hiding this comment.
Use one Actuator.step() calling convention throughout this page.
Lines 95-96 show actuator.step(state, control), while Line 127 shows pid_actuator.step(sim_state, sim_control, current_state, next_state, dt=0.01). These cannot both be the public API, so the page currently teaches at least one broken example.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/concepts/actuators.rst` around lines 87 - 128, The page mixes two
different Actuator.step() call styles; unify to the full public signature used
by stateful actuators (e.g., Actuator.step(sim_state, sim_control,
current_state, next_state, dt)) and update both examples to use that exact
ordering: change the simple loop to call actuator.step(sim_state, sim_control,
None, None, dt) and keep the PID example calling pid_actuator.step(sim_state,
sim_control, current_state, next_state, dt=0.01); ensure references to
ActuatorPID.is_stateful() and ActuatorPID.state() remain to show when to create
and supply current_state/next_state.
Comment on lines
+104
to
+116
| from newton.actuators import ActuatorPID | ||
|
|
||
| indices = wp.array([0, 1], dtype=wp.uint32) | ||
| pid_actuator = ActuatorPID( | ||
| input_indices=indices, | ||
| output_indices=indices, | ||
| kp=wp.array([100.0, 100.0], dtype=wp.float32), | ||
| ki=wp.array([10.0, 10.0], dtype=wp.float32), | ||
| kd=wp.array([5.0, 5.0], dtype=wp.float32), | ||
| max_force=wp.array([50.0, 50.0], dtype=wp.float32), | ||
| integral_max=wp.array([10.0, 10.0], dtype=wp.float32), | ||
| constant_force=wp.array([0.0, 0.0], dtype=wp.float32), | ||
| ) |
Contributor
There was a problem hiding this comment.
Import Warp in the stateful example.
The snippet starts constructing wp.array(...) on Line 106, but wp is never imported in that block.
Suggested fix
+ import warp as wp
from newton.actuators import ActuatorPID📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from newton.actuators import ActuatorPID | |
| indices = wp.array([0, 1], dtype=wp.uint32) | |
| pid_actuator = ActuatorPID( | |
| input_indices=indices, | |
| output_indices=indices, | |
| kp=wp.array([100.0, 100.0], dtype=wp.float32), | |
| ki=wp.array([10.0, 10.0], dtype=wp.float32), | |
| kd=wp.array([5.0, 5.0], dtype=wp.float32), | |
| max_force=wp.array([50.0, 50.0], dtype=wp.float32), | |
| integral_max=wp.array([10.0, 10.0], dtype=wp.float32), | |
| constant_force=wp.array([0.0, 0.0], dtype=wp.float32), | |
| ) | |
| import warp as wp | |
| from newton.actuators import ActuatorPID | |
| indices = wp.array([0, 1], dtype=wp.uint32) | |
| pid_actuator = ActuatorPID( | |
| input_indices=indices, | |
| output_indices=indices, | |
| kp=wp.array([100.0, 100.0], dtype=wp.float32), | |
| ki=wp.array([10.0, 10.0], dtype=wp.float32), | |
| kd=wp.array([5.0, 5.0], dtype=wp.float32), | |
| max_force=wp.array([50.0, 50.0], dtype=wp.float32), | |
| integral_max=wp.array([10.0, 10.0], dtype=wp.float32), | |
| constant_force=wp.array([0.0, 0.0], dtype=wp.float32), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/concepts/actuators.rst` around lines 104 - 116, The example uses
wp.array(...) but never imports Warp; add the missing import (e.g., "import warp
as wp") at the top of the example block before any wp.array calls so the
ActuatorPID example (symbols: ActuatorPID, wp.array, pid_actuator) can run;
ensure the import appears alongside the other imports in that snippet.
Comment on lines
+147
to
+149
| Actuator prims in USD stages are automatically parsed during | ||
| :func:`newton.usd.parse_usd`. The parser inspects | ||
| ``newton:actuator:*`` attributes to infer the actuator class and parameters. |
Contributor
There was a problem hiding this comment.
Reference the public USD entry point here.
Linking to :func:\newton.usd.parse_usd`points readers at a private helper and likely an unresolved Sphinx target. Prefer:meth:`~newton.ModelBuilder.add_usd`` or plain parse_usd() if you only want to mention the implementation detail.
Based on learnings, parse_usd in newton/_src/utils/import_usd.py is a private helper and is not re-exported as part of the public API (the public entry point is ModelBuilder.add_usd).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/concepts/actuators.rst` around lines 147 - 149, The docs currently
reference the private helper `newton.usd.parse_usd`; update the sentence to
reference the public entry point instead by replacing the Sphinx link
`:func:\`newton.usd.parse_usd\`` with `:meth:\`~newton.ModelBuilder.add_usd\``
(or use plain ``parse_usd()`` if you prefer a non-linked mention), and ensure
the text clarifies that actuator prims are parsed via the public
`ModelBuilder.add_usd` path rather than the private `parse_usd` implementation.
Comment on lines
+97
to
+117
| for name, arr in [ | ||
| ("kp", kp), | ||
| ("kd", kd), | ||
| ("max_force", max_force), | ||
| ("saturation_effort", saturation_effort), | ||
| ("velocity_limit", velocity_limit), | ||
| ]: | ||
| if len(arr) != self.num_actuators: | ||
| raise ValueError(f"{name} length ({len(arr)}) must match num_actuators ({self.num_actuators})") | ||
|
|
||
| if constant_force is not None and len(constant_force) != self.num_actuators: | ||
| raise ValueError( | ||
| f"constant_force length ({len(constant_force)}) must match num_actuators ({self.num_actuators})" | ||
| ) | ||
|
|
||
| self.kp = kp | ||
| self.kd = kd | ||
| self.max_force = max_force | ||
| self.saturation_effort = saturation_effort | ||
| self.velocity_limit = velocity_limit | ||
| self.constant_force = constant_force |
Contributor
There was a problem hiding this comment.
Reject non-positive velocity_limit values in the constructor.
The parser tests already expect velocity_limit <= 0 to raise, but the direct constructor path only checks length. Zero or negative entries reach the torque-speed division in pd_controller_kernel, which can produce inf/NaN or flip the saturation curve.
Suggested fix
if constant_force is not None and len(constant_force) != self.num_actuators:
raise ValueError(
f"constant_force length ({len(constant_force)}) must match num_actuators ({self.num_actuators})"
)
+ if any(v <= 0.0 for v in velocity_limit.numpy()):
+ raise ValueError("velocity_limit values must be > 0")
self.kp = kp
self.kd = kd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/actuators/dc_motor.py` around lines 97 - 117, The
constructor currently only checks array lengths (in the loop iterating over
"kp","kd","max_force","saturation_effort","velocity_limit") but does not
validate values; add a guard in the DC motor constructor to reject non-positive
entries in velocity_limit by iterating over velocity_limit and raising a
ValueError (with a clear message including the offending value/index and
expected >0) if any entry <= 0; reference the velocity_limit validation in the
constructor (and mention pd_controller_kernel uses velocity_limit for
torque-speed division) so callers can't pass zeros or negatives that later cause
inf/NaN or incorrect saturation behavior.
Comment on lines
+78
to
+126
| def __init__( | ||
| self, | ||
| input_indices: wp.array[wp.uint32], | ||
| output_indices: wp.array[wp.uint32], | ||
| kp: wp.array[float], | ||
| kd: wp.array[float], | ||
| delay: int, | ||
| max_force: wp.array[float], | ||
| constant_force: wp.array[float] | None = None, | ||
| state_pos_attr: str = "joint_q", | ||
| state_vel_attr: str = "joint_qd", | ||
| control_target_pos_attr: str = "joint_target_pos", | ||
| control_target_vel_attr: str = "joint_target_vel", | ||
| control_input_attr: str = "joint_act", | ||
| control_output_attr: str = "joint_f", | ||
| ): | ||
| """Initialize delayed PD actuator. | ||
|
|
||
| Args: | ||
| input_indices: DOF indices for reading state and targets. Shape ``(N,)``. | ||
| output_indices: DOF indices for writing output. Shape ``(N,)``. | ||
| kp: Proportional gains. Shape ``(N,)``. | ||
| kd: Derivative gains. Shape ``(N,)``. | ||
| delay: Number of timesteps to delay inputs. | ||
| max_force: Force limits [N or N·m]. Shape ``(N,)``. | ||
| constant_force: Constant offsets [N or N·m]. Shape ``(N,)``. ``None`` to skip. | ||
| state_pos_attr: Attribute on :class:`~newton.State` for positions. | ||
| state_vel_attr: Attribute on :class:`~newton.State` for velocities. | ||
| control_target_pos_attr: Attribute on :class:`~newton.Control` for target positions. | ||
| control_target_vel_attr: Attribute on :class:`~newton.Control` for target velocities. | ||
| control_input_attr: Attribute on :class:`~newton.Control` for control input. ``None`` to skip. | ||
| control_output_attr: Attribute on :class:`~newton.Control` for output forces. | ||
| """ | ||
| super().__init__(input_indices, output_indices, control_output_attr) | ||
|
|
||
| for name, arr in [("kp", kp), ("kd", kd), ("max_force", max_force)]: | ||
| if len(arr) != self.num_actuators: | ||
| raise ValueError(f"{name} length ({len(arr)}) must match num_actuators ({self.num_actuators})") | ||
|
|
||
| if constant_force is not None and len(constant_force) != self.num_actuators: | ||
| raise ValueError( | ||
| f"constant_force length ({len(constant_force)}) must match num_actuators ({self.num_actuators})" | ||
| ) | ||
|
|
||
| self.kp = kp | ||
| self.kd = kd | ||
| self.max_force = max_force | ||
| self.constant_force = constant_force | ||
| self.delay = delay |
Contributor
There was a problem hiding this comment.
Reject delay <= 0 before building buffers.
With a zero or negative delay, state() allocates zero/invalid buffers and Lines 147 and 193 hit modulo-by-zero during stepping. This should fail in __init__() instead of surfacing later as a runtime error.
Proposed fix
def __init__(
self,
input_indices: wp.array[wp.uint32],
output_indices: wp.array[wp.uint32],
kp: wp.array[float],
@@
):
"""Initialize delayed PD actuator.
@@
"""
super().__init__(input_indices, output_indices, control_output_attr)
+
+ if delay < 1:
+ raise ValueError(f"delay must be >= 1; got {delay}")
for name, arr in [("kp", kp), ("kd", kd), ("max_force", max_force)]:
if len(arr) != self.num_actuators:
raise ValueError(f"{name} length ({len(arr)}) must match num_actuators ({self.num_actuators})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/actuators/delayed_pd.py` around lines 78 - 126, The
constructor (__init__) must validate the delay parameter and reject non-positive
values to avoid downstream buffer allocation and modulo-by-zero errors in
state() and the actuator stepping logic; add a check near the top of __init__
(before any buffer/state setup or assignment to self.delay) that raises
ValueError if delay < 1 (e.g., "raise ValueError('delay must be >= 1')"),
ensuring callers cannot construct DelayedPD actuators with delay <= 0 which
later cause modulo-by-zero in methods that index using self.delay.
Comment on lines
+122
to
+135
| if len(lookup_angles) != len(lookup_torques): | ||
| raise ValueError( | ||
| f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})" | ||
| ) | ||
|
|
||
| device = input_indices.device | ||
| if not isinstance(lookup_angles, wp.array): | ||
| lookup_angles = wp.array(np.array(lookup_angles, dtype=np.float32), device=device) | ||
| if not isinstance(lookup_torques, wp.array): | ||
| lookup_torques = wp.array(np.array(lookup_torques, dtype=np.float32), device=device) | ||
|
|
||
| self.lookup_angles = lookup_angles | ||
| self.lookup_torques = lookup_torques | ||
| self.lookup_size = len(lookup_angles) |
Contributor
There was a problem hiding this comment.
Validate that lookup_angles is strictly increasing.
The docstring says the table must be sorted, but the constructor only checks matching lengths. Unsorted or repeated angles will make the interpolation path return wrong torque limits silently.
Suggested fix
if len(lookup_angles) != len(lookup_torques):
raise ValueError(
f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})"
)
+ lookup_angles_np = (
+ lookup_angles.numpy()
+ if isinstance(lookup_angles, wp.array)
+ else np.asarray(lookup_angles, dtype=np.float32)
+ )
+ if lookup_angles_np.size == 0 or np.any(np.diff(lookup_angles_np) <= 0.0):
+ raise ValueError("lookup_angles must be a non-empty, strictly increasing sequence")
device = input_indices.device
if not isinstance(lookup_angles, wp.array):
- lookup_angles = wp.array(np.array(lookup_angles, dtype=np.float32), device=device)
+ lookup_angles = wp.array(lookup_angles_np, device=device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/actuators/remotized_pd.py` around lines 122 - 135, In
the RemotizedPD constructor in remotized_pd.py, after converting lookup_angles
to a wp.array (before assigning self.lookup_angles), validate that the table is
strictly increasing by checking consecutive differences are all > 0 (e.g.,
compute diffs of lookup_angles and verify no diff <= 0); if any non-positive
difference is found, raise a ValueError stating that lookup_angles must be
strictly increasing (include lengths/indices for clarity). Ensure this check
runs on the wp.array form you assign to self.lookup_angles so unsorted or
repeated angles are rejected early.
Comment on lines
+83
to
+90
| if saturation_effort: | ||
| vel = current_vel[state_idx] | ||
| sat = saturation_effort[i] | ||
| vel_lim = velocity_limit[i] | ||
| max_f = max_force[i] | ||
| max_torque = wp.clamp(sat * (1.0 - vel / vel_lim), 0.0, max_f) | ||
| min_torque = wp.clamp(sat * (-1.0 - vel / vel_lim), -max_f, 0.0) | ||
| force = wp.clamp(force, min_torque, max_torque) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if velocity_limit is validated elsewhere when saturation_effort is used
rg -n "velocity_limit" --type=py -C3 | head -100Repository: newton-physics/newton
Length of output: 7761
🏁 Script executed:
# Read the kernel file to see the context around lines 83-90
cat -n newton/_src/actuators/kernels.py | sed -n '75,100p'Repository: newton-physics/newton
Length of output: 1127
🏁 Script executed:
# Search for velocity_limit initialization and validation in the codebase
rg -n "velocity_limit\s*=" --type=py -B2 -A2 | head -80Repository: newton-physics/newton
Length of output: 6811
🏁 Script executed:
# Look for any guards or checks related to velocity_limit or saturation_effort
rg -n "saturation_effort" --type=py -B3 -A8 | head -150Repository: newton-physics/newton
Length of output: 10979
🏁 Script executed:
# Search for the validation that raises ValueError for velocity_limit=0
rg -n "velocity_limit.*zero\|velocity_limit.*0\|ValueError.*velocity" --type=py -B3 -A3 | head -100Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Look at the DC motor actuator implementation
fd -e py "dc_motor\|dcmotor" --type f | head -20Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Search in the actuators directory for resolve_arguments or validation
rg -n "resolve_arguments\|velocity_limit" newton/_src/actuators --type=py | head -80Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Look at the test that checks for velocity_limit=0 error
cat -n newton/tests/test_actuators.py | sed -n '754,780p'Repository: newton-physics/newton
Length of output: 1435
🏁 Script executed:
# Search for DCMotor class definition
fd -e py --type f | xargs grep -l "class.*DCMotor\|class ActuatorDCMotor" | head -5Repository: newton-physics/newton
Length of output: 139
🏁 Script executed:
# Look for any validation in actuators init or base classes
rg -n "__init__\|resolve_arguments" newton/_src/actuators/actuators --type=py -A 10 | head -150Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Read the DC motor implementation to find validation
cat -n newton/_src/actuators/actuators/dc_motor.py | head -100Repository: newton-physics/newton
Length of output: 4813
🏁 Script executed:
# Search for the ValueError raise related to velocity_limit
rg -n "ValueError\|velocity_limit" newton/_src/actuators/actuators/dc_motor.py -B2 -A2Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Also check the parser for validation
rg -n "velocity_limit\|ValueError" newton/_src/actuators/usd_parser.py -B2 -A2Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Continue reading dc_motor.py to see the full __init__ and any validation
cat -n newton/_src/actuators/actuators/dc_motor.py | sed -n '95,150p'Repository: newton-physics/newton
Length of output: 2643
🏁 Script executed:
# Search for ValueError in the entire actuators module
rg -n "ValueError" newton/_src/actuators --type=py -B2 -A1Repository: newton-physics/newton
Length of output: 11928
🏁 Script executed:
# Get the full error message in usd_parser.py for velocity_limit validation
cat -n newton/_src/actuators/usd_parser.py | sed -n '109,120p'Repository: newton-physics/newton
Length of output: 693
🏁 Script executed:
# Check if there's any validation in ActuatorDCMotor.__init__ for velocity_limit values
cat -n newton/_src/actuators/actuators/dc_motor.py | sed -n '95,125p'Repository: newton-physics/newton
Length of output: 1502
🏁 Script executed:
# Check if parse_actuator_prim calls validate_kwargs
rg -n "parse_actuator_prim\|validate_kwargs" newton/_src/actuators/usd_parser.py -A5 -B2 | head -80Repository: newton-physics/newton
Length of output: 47
🏁 Script executed:
# Check if there's any value-level validation in ActuatorDCMotor after construction
rg -n "velocity_limit\s*>" newton/_src/actuators --type=pyRepository: newton-physics/newton
Length of output: 191
🏁 Script executed:
# Check the full parse_actuator_prim function to see if validate_kwargs is called
cat -n newton/_src/actuators/usd_parser.py | sed -n '120,200p'Repository: newton-physics/newton
Length of output: 1590
🏁 Script executed:
# Check if resolve_arguments in dc_motor.py does any value validation
cat -n newton/_src/actuators/actuators/dc_motor.py | sed -n '36,60p'Repository: newton-physics/newton
Length of output: 1071
Add validation for velocity_limit > 0 in ActuatorDCMotor initialization.
The kernel divides by velocity_limit at lines 88–89. While the USD parser validates this (usd_parser.py:113–117), direct Python construction bypasses that check—if someone calls ActuatorDCMotor(..., velocity_limit=wp.array([0.0])), the kernel will produce inf/nan at runtime.
Add the constraint check to ActuatorDCMotor.resolve_arguments() or __init__() to catch zero/negative values regardless of construction path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/kernels.py` around lines 83 - 90, Add a runtime
validation to ensure all entries of velocity_limit are > 0 when an
ActuatorDCMotor is constructed; update ActuatorDCMotor.resolve_arguments (or its
__init__) to check the velocity_limit array passed in and raise a clear
ValueError if any value is <= 0 so the kernel lines using velocity_limit (where
max_torque/min_torque compute sat * (1.0 - vel / vel_lim)) cannot divide by
zero; reference the ActuatorDCMotor class and its resolve_arguments method to
locate where to perform this check and include the failing value(s) in the error
message.
Comment on lines
+18
to
+19
| kwargs: dict[str, Any] = field(default_factory=dict) | ||
| transmission: list[float] | None = None |
Contributor
There was a problem hiding this comment.
Parsed transmission is dropped on the importer path.
parse_actuator_prim() returns the authored newton:actuator:transmission, but newton/_src/utils/import_usd.py Lines 2755-2770 only forwards parsed.kwargs into builder.add_actuator(...). As written, USD transmission data is parsed and then ignored during actuator construction. Please either fold it into kwargs or update the importer contract so this field is actually consumed.
Also applies to: 146-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/usd_parser.py` around lines 18 - 19, The parsed
transmission field (transmission on the actuator data object returned by
parse_actuator_prim) is being dropped because import_usd's call to
builder.add_actuator(...) only forwards parsed.kwargs; modify the importer to
include parsed.transmission when constructing the actuator (e.g., fold it into
parsed.kwargs under a clear key like "transmission" before calling
builder.add_actuator, or change the add_actuator call to accept a
transmission=parsed.transmission parameter) so that the transmission data
produced by parse_actuator_prim is actually consumed; update any related call
sites and type hints for add_actuator/kwargs to reflect this new parameter
usage.
Comment on lines
+2761
to
+2768
| for prim in Usd.PrimRange(stage.GetPrimAtPath(root_path)): | ||
| parsed = parse_actuator_prim(prim) | ||
| if parsed is None: | ||
| continue | ||
| dof_indices = [path_to_dof[p] for p in parsed.target_paths if p in path_to_dof] | ||
| if dof_indices: | ||
| builder.add_actuator(parsed.actuator_class, input_indices=dof_indices, **parsed.kwargs) | ||
| actuator_count += 1 |
Contributor
There was a problem hiding this comment.
Don't silently discard authored actuator transmission.
parse_actuator_prim() returns a transmission vector, and the new parser tests assert that field, but this importer never threads it into add_actuator() or rejects it. A USD file can therefore author newton:actuator:transmission, parse cleanly, and still run with default 1:1 actuation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/utils/import_usd.py` around lines 2761 - 2768, The importer loop
is dropping the parsed actuator's transmission vector; update the code in the
loop that calls parse_actuator_prim and builder.add_actuator so that any
transmission returned by parse_actuator_prim (e.g. parsed.transmission) is
threaded into add_actuator — either by adding transmission=parsed.transmission
to the call or by inserting it into parsed.kwargs before calling
builder.add_actuator; if a transmission is present but invalid, explicitly
reject or raise a clear error instead of silently ignoring it.
Comment on lines
+56
to
+65
| def test_pd_actuator_creation(self): | ||
| """Test that ActuatorPD can be created with valid parameters.""" | ||
| indices = wp.array([0, 1, 2], dtype=wp.uint32) | ||
| actuator = ActuatorPD( | ||
| input_indices=indices, | ||
| output_indices=indices, | ||
| kp=wp.array([100.0, 100.0, 100.0], dtype=wp.float32), | ||
| kd=wp.array([10.0, 10.0, 10.0], dtype=wp.float32), | ||
| max_force=wp.array([50.0, 50.0, 50.0], dtype=wp.float32), | ||
| ) |
Contributor
There was a problem hiding this comment.
These PD tests pass a kwarg that ActuatorPD does not accept.
newton/_src/actuators/actuators/pd.py defines ActuatorPD.__init__(..., kp, kd, constant_force=None, ...) with no max_force parameter. Both tests will fail during construction before they exercise any controller behavior.
Suggested fix
actuator = ActuatorPD(
input_indices=indices,
output_indices=indices,
kp=wp.array([100.0, 100.0, 100.0], dtype=wp.float32),
kd=wp.array([10.0, 10.0, 10.0], dtype=wp.float32),
- max_force=wp.array([50.0, 50.0, 50.0], dtype=wp.float32),
)
@@
actuator = ActuatorPD(
input_indices=indices,
output_indices=indices,
kp=wp.array([100.0, 100.0, 100.0], dtype=wp.float32),
kd=wp.array([0.0, 0.0, 0.0], dtype=wp.float32),
- max_force=wp.array([1000.0, 1000.0, 1000.0], dtype=wp.float32),
)Also applies to: 76-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/tests/test_actuators.py` around lines 56 - 65, Tests pass a kwarg
named max_force to ActuatorPD, but ActuatorPD.__init__ accepts constant_force
(no max_force), causing construction to fail; update the tests (e.g.,
test_pd_actuator_creation and the similar test around the next case) to replace
max_force=... with constant_force=... (use the same wp.array values) so the
constructor matches ActuatorPD(input_indices, output_indices, kp, kd,
constant_force=...) or alternatively add a max_force parameter to ActuatorPD
that maps to the existing constant_force internally, but prefer updating the
tests for minimal change.
Fixes all 26 ruff lint errors introduced by the actuator integration: - RUF002: Replace en dash (U+2013) with double hyphen in base.py docstring (3 occurrences) - RUF012: Annotate mutable SCALAR_PARAMS class attributes with ClassVar in base.py, delayed_pd.py, net_lstm.py, net_mlp.py, remotized_pd.py (5 occurrences) - RUF013: Fix implicit Optional — dt: float = None -> dt: float | None = None in base.py (1 occurrence) - PLC0415: Move 17 per-method imports in test_actuators.py to top-level (ActuatorNetMLP, ActuatorNetLSTM, parse_actuator_prim were redundantly imported inside each test method) - Remove stale gear parameter references from integration tests — gear was supported by the old newton_actuators package but is not implemented in the new in-tree actuator module. The test_accumulation_and_parameters test passed gear=2.5 (silently dropped by resolve_arguments) then asserted act.gear which doesn't exist. The test_pd_step_gear_and_clamp test was converted to test_pd_step_clamp testing pure max_force clamping.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
newton/_src/actuators/actuators/delayed_pd.py (1)
111-126:⚠️ Potential issue | 🟠 MajorReject non-positive delays up front.
delay == 0creates zero-length buffers and then Line 193 hits modulo-by-zero during stepping; negative values fail even earlier in buffer allocation. This should be a constructor error, not a runtime surprise.Suggested fix
super().__init__(input_indices, output_indices, control_output_attr) + + if delay < 1: + raise ValueError(f"delay must be >= 1; got {delay}") for name, arr in [("kp", kp), ("kd", kd), ("max_force", max_force)]: if len(arr) != self.num_actuators: raise ValueError(f"{name} length ({len(arr)}) must match num_actuators ({self.num_actuators})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/actuators/delayed_pd.py` around lines 111 - 126, The constructor currently accepts non-positive delay causing zero-length buffers and modulo-by-zero during step; in the DelayedPD constructor (where super().__init__ is called and self.delay is set) validate that delay is a positive integer and raise a ValueError if delay <= 0 with a clear message referencing delay and self.num_actuators; ensure this check runs before any buffer allocation or use of self.delay so zero or negative delays are rejected up front.newton/_src/actuators/actuators/net_mlp.py (1)
42-85:⚠️ Potential issue | 🟠 Major
SCALAR_PARAMSis still missing the MLP input-layout knobs.
pos_scale,vel_scale,torque_scale,input_order, andinput_idxare all consumed once per actuator instance in__init__(). Leaving them out lets batching merge incompatibleActuatorNetMLPconfigs, andinput_idxshould be normalized to a tuple if it participates in the key.Suggested fix
- SCALAR_PARAMS: ClassVar[set[str]] = {"network_path"} + SCALAR_PARAMS: ClassVar[set[str]] = { + "network_path", + "pos_scale", + "vel_scale", + "torque_scale", + "input_order", + "input_idx", + } @@ return { "network_path": args["network_path"], "max_force": args.get("max_force", math.inf), "pos_scale": args.get("pos_scale", 1.0), "vel_scale": args.get("vel_scale", 1.0), "torque_scale": args.get("torque_scale", 1.0), "input_order": args.get("input_order", "pos_vel"), - "input_idx": args.get("input_idx", None), + "input_idx": tuple(args["input_idx"]) if args.get("input_idx") is not None else (0,), } @@ - input_idx: list[int] | None = None, + input_idx: list[int] | tuple[int, ...] | None = None,Also applies to: 87-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/actuators/net_mlp.py` around lines 42 - 85, SCALAR_PARAMS must include the per-instance MLP knobs so batching doesn't merge incompatible configs: add "pos_scale", "vel_scale", "torque_scale", "input_order", and "input_idx" to the SCALAR_PARAMS set next to "network_path"; also normalize input_idx into a canonical, hashable form (e.g., tuple or None) inside resolve_arguments (or at start of __init__) so the value used in keys is consistent across callers and suitable for batching/lookups. Reference: SCALAR_PARAMS, resolve_arguments, __init__, and input_idx.newton/_src/actuators/actuators/remotized_pd.py (1)
122-135:⚠️ Potential issue | 🟠 MajorValidate the lookup table before storing it.
pd_controller_kernel's interpolation path assumes a non-empty, strictly increasinglookup_anglestable. Repeated or unsorted angles return wrong limits, and an empty table falls back to the dummymax_force=inf, effectively removing torque limiting.Suggested fix
if len(lookup_angles) != len(lookup_torques): raise ValueError( f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})" ) + lookup_angles_np = ( + lookup_angles.numpy() + if isinstance(lookup_angles, wp.array) + else np.asarray(lookup_angles, dtype=np.float32) + ) + if lookup_angles_np.size == 0 or np.any(np.diff(lookup_angles_np) <= 0.0): + raise ValueError("lookup_angles must be a non-empty, strictly increasing sequence") device = input_indices.device if not isinstance(lookup_angles, wp.array): - lookup_angles = wp.array(np.array(lookup_angles, dtype=np.float32), device=device) + lookup_angles = wp.array(lookup_angles_np, device=device) if not isinstance(lookup_torques, wp.array): lookup_torques = wp.array(np.array(lookup_torques, dtype=np.float32), device=device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/actuators/remotized_pd.py` around lines 122 - 135, The lookup table is not validated for emptiness or monotonicity which pd_controller_kernel expects; before assigning self.lookup_angles, self.lookup_torques and self.lookup_size validate that lookup_angles/lookup_torques are non-empty and that lookup_angles is strictly increasing (no duplicates/unsorted values) after converting to a wp.array on the target device; if the checks fail, raise a ValueError with a clear message referencing lookup_angles/lookup_torques so callers know to provide a non-empty, strictly increasing angle table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/actuators/base.py`:
- Around line 150-175: The step() method allows missing state buffers for
stateful actuators, causing crashes or silent failures (e.g., ActuatorDelayedPD
expects current_act_state, ActuatorNetMLP/ActuatorNetLSTM expect
next_act_state); update base Actuator.step() to validate that when
self.is_stateful() is True both current_act_state and next_act_state are not
None and raise a clear ValueError if either is missing before calling
_run_controller or _run_state_manager; include references to
current_act_state/next_act_state in the error message so callers can fix their
buffers.
- Around line 66-71: The base class currently enforces len(output_indices) ==
len(input_indices) (using self.num_actuators), which rejects valid actuator
mappings; remove that equality check from the base class and defer one-to-one
validation to concrete actuator implementations that rely on output_indices[i],
or replace it with a more general validation (e.g., ensure output_indices is not
None/empty or validate row counts where appropriate). Update the initializer
around self.num_actuators, input_indices, and output_indices to stop raising
ValueError here and add a clarifying comment that specific actuators (those
indexing output_indices by i) must perform their own length checks.
---
Duplicate comments:
In `@newton/_src/actuators/actuators/delayed_pd.py`:
- Around line 111-126: The constructor currently accepts non-positive delay
causing zero-length buffers and modulo-by-zero during step; in the DelayedPD
constructor (where super().__init__ is called and self.delay is set) validate
that delay is a positive integer and raise a ValueError if delay <= 0 with a
clear message referencing delay and self.num_actuators; ensure this check runs
before any buffer allocation or use of self.delay so zero or negative delays are
rejected up front.
In `@newton/_src/actuators/actuators/net_mlp.py`:
- Around line 42-85: SCALAR_PARAMS must include the per-instance MLP knobs so
batching doesn't merge incompatible configs: add "pos_scale", "vel_scale",
"torque_scale", "input_order", and "input_idx" to the SCALAR_PARAMS set next to
"network_path"; also normalize input_idx into a canonical, hashable form (e.g.,
tuple or None) inside resolve_arguments (or at start of __init__) so the value
used in keys is consistent across callers and suitable for batching/lookups.
Reference: SCALAR_PARAMS, resolve_arguments, __init__, and input_idx.
In `@newton/_src/actuators/actuators/remotized_pd.py`:
- Around line 122-135: The lookup table is not validated for emptiness or
monotonicity which pd_controller_kernel expects; before assigning
self.lookup_angles, self.lookup_torques and self.lookup_size validate that
lookup_angles/lookup_torques are non-empty and that lookup_angles is strictly
increasing (no duplicates/unsorted values) after converting to a wp.array on the
target device; if the checks fail, raise a ValueError with a clear message
referencing lookup_angles/lookup_torques so callers know to provide a non-empty,
strictly increasing angle table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1be2f8ba-a650-45a2-9d03-0547155d90f3
📒 Files selected for processing (7)
newton/_src/actuators/actuators/base.pynewton/_src/actuators/actuators/delayed_pd.pynewton/_src/actuators/actuators/net_lstm.pynewton/_src/actuators/actuators/net_mlp.pynewton/_src/actuators/actuators/remotized_pd.pynewton/tests/test_actuators.pynewton/tests/test_actuators_integration.py
✅ Files skipped from review due to trivial changes (1)
- newton/tests/test_actuators_integration.py
Comment on lines
+66
to
+71
| self.num_actuators = len(input_indices) | ||
|
|
||
| if len(output_indices) != self.num_actuators: | ||
| raise ValueError( | ||
| f"output_indices length ({len(output_indices)}) must match input_indices length ({self.num_actuators})" | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't hard-code equal input/output lengths in the base class.
This rejects valid actuator mappings before the concrete actuator can interpret its own indexing scheme. Keep one-to-one validation in actuators that index output_indices[i] directly, or validate row count instead of raw len().
Based on learnings: Repo: newton-physics/newton; File: newton/_src/sim/builder.py (ModelBuilder.add_actuator). Input and output index lists are allowed to have different lengths; do not enforce equality in reviews.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/actuators/base.py` around lines 66 - 71, The base class
currently enforces len(output_indices) == len(input_indices) (using
self.num_actuators), which rejects valid actuator mappings; remove that equality
check from the base class and defer one-to-one validation to concrete actuator
implementations that rely on output_indices[i], or replace it with a more
general validation (e.g., ensure output_indices is not None/empty or validate
row counts where appropriate). Update the initializer around self.num_actuators,
input_indices, and output_indices to stop raising ValueError here and add a
clarifying comment that specific actuators (those indexing output_indices by i)
must perform their own length checks.
Comment on lines
+150
to
+175
| current_act_state: Any = None, | ||
| next_act_state: Any = None, | ||
| dt: float | None = None, | ||
| ) -> None: | ||
| """Execute one control step. | ||
|
|
||
| Args: | ||
| sim_state: Simulation state. | ||
| sim_control: Control structure. | ||
| current_act_state: Current internal state (``None`` if stateless). | ||
| next_act_state: Next internal state (``None`` if stateless). | ||
| dt: Time step [s]. | ||
| """ | ||
| if self.has_transmission(): | ||
| controller_output = self._actuation_forces | ||
| controller_output_indices = self._sequential_indices | ||
| else: | ||
| controller_output = getattr(sim_control, self.control_output_attr) | ||
| controller_output_indices = self.output_indices | ||
|
|
||
| self._run_controller( | ||
| sim_state, sim_control, controller_output, controller_output_indices, current_act_state, dt | ||
| ) | ||
|
|
||
| if self.is_stateful(): | ||
| self._run_state_manager(sim_state, sim_control, current_act_state, next_act_state, dt) |
Contributor
There was a problem hiding this comment.
Require both state buffers before stepping a stateful actuator.
The docstring already makes None the stateless case, but step() doesn't enforce it. ActuatorDelayedPD will blow up when current_act_state is missing, while ActuatorNetMLP and ActuatorNetLSTM silently stop propagating state when next_act_state is None.
Suggested fix
if self.has_transmission():
controller_output = self._actuation_forces
controller_output_indices = self._sequential_indices
else:
controller_output = getattr(sim_control, self.control_output_attr)
controller_output_indices = self.output_indices
+
+ if self.is_stateful() and (current_act_state is None or next_act_state is None):
+ raise ValueError(
+ f"{type(self).__name__}.step() requires both current_act_state and next_act_state"
+ )
self._run_controller(
sim_state, sim_control, controller_output, controller_output_indices, current_act_state, dt
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| current_act_state: Any = None, | |
| next_act_state: Any = None, | |
| dt: float | None = None, | |
| ) -> None: | |
| """Execute one control step. | |
| Args: | |
| sim_state: Simulation state. | |
| sim_control: Control structure. | |
| current_act_state: Current internal state (``None`` if stateless). | |
| next_act_state: Next internal state (``None`` if stateless). | |
| dt: Time step [s]. | |
| """ | |
| if self.has_transmission(): | |
| controller_output = self._actuation_forces | |
| controller_output_indices = self._sequential_indices | |
| else: | |
| controller_output = getattr(sim_control, self.control_output_attr) | |
| controller_output_indices = self.output_indices | |
| self._run_controller( | |
| sim_state, sim_control, controller_output, controller_output_indices, current_act_state, dt | |
| ) | |
| if self.is_stateful(): | |
| self._run_state_manager(sim_state, sim_control, current_act_state, next_act_state, dt) | |
| current_act_state: Any = None, | |
| next_act_state: Any = None, | |
| dt: float | None = None, | |
| ) -> None: | |
| """Execute one control step. | |
| Args: | |
| sim_state: Simulation state. | |
| sim_control: Control structure. | |
| current_act_state: Current internal state (``None`` if stateless). | |
| next_act_state: Next internal state (``None`` if stateless). | |
| dt: Time step [s]. | |
| """ | |
| if self.has_transmission(): | |
| controller_output = self._actuation_forces | |
| controller_output_indices = self._sequential_indices | |
| else: | |
| controller_output = getattr(sim_control, self.control_output_attr) | |
| controller_output_indices = self.output_indices | |
| if self.is_stateful() and (current_act_state is None or next_act_state is None): | |
| raise ValueError( | |
| f"{type(self).__name__}.step() requires both current_act_state and next_act_state" | |
| ) | |
| self._run_controller( | |
| sim_state, sim_control, controller_output, controller_output_indices, current_act_state, dt | |
| ) | |
| if self.is_stateful(): | |
| self._run_state_manager(sim_state, sim_control, current_act_state, next_act_state, dt) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/actuators/actuators/base.py` around lines 150 - 175, The step()
method allows missing state buffers for stateful actuators, causing crashes or
silent failures (e.g., ActuatorDelayedPD expects current_act_state,
ActuatorNetMLP/ActuatorNetLSTM expect next_act_state); update base
Actuator.step() to validate that when self.is_stateful() is True both
current_act_state and next_act_state are not None and raise a clear ValueError
if either is missing before calling _run_controller or _run_state_manager;
include references to current_act_state/next_act_state in the error message so
callers can fix their buffers.
|
|
||
| def parse_actuator_prim(prim) -> ParsedActuator | None: | ||
| """Parse a USD Actuator prim. Returns ``None`` if not a valid actuator.""" | ||
| if prim.GetTypeName() != "Actuator": |
Member
There was a problem hiding this comment.
I know this is a port of what we already have, but I just want to flag again
- There will be no prim type
Actuator, it would beNewtonActuator(and similarly all applied APIs would have a Newton prefix) - It seems odd to me to include the USD parsing of the schemas before we have the USD schemas.
|
|
||
| Actuator prims in USD stages are automatically parsed during | ||
| :func:`newton.usd.parse_usd`. The parser inspects | ||
| ``newton:actuator:*`` attributes to infer the actuator class and parameters. |
Member
There was a problem hiding this comment.
It should inspect the applied APIs to infer the actuator class & then the newton:* attributes to populate the class parameters
| transmission: list[float] | None = None | ||
|
|
||
|
|
||
| API_SCHEMA_HANDLERS: dict[str, dict[str, str]] = { |
Member
There was a problem hiding this comment.
Ideally this would all land in the SchemaResolvers I think? Maybe that's overkill, not sure really...
| } | ||
|
|
||
|
|
||
| def get_attribute(prim, name: str, default: Any = None) -> Any: |
Member
There was a problem hiding this comment.
some of these must be common utils
| return names | ||
|
|
||
|
|
||
| def infer_schemas_from_prim(prim) -> list[str]: |
Member
There was a problem hiding this comment.
This shouldn't be necessary, we should be able to just use HasAPI
| if TYPE_CHECKING: | ||
| from newton_actuators import Actuator | ||
|
|
||
| from ..actuators.actuators.base import Actuator |
Member
There was a problem hiding this comment.
why do we have this doubled namespace?
Comment on lines
+22
to
+23
| ParsedActuator, | ||
| parse_actuator_prim, |
Member
There was a problem hiding this comment.
Are these 2 meant to be public symbols?
Member
Author
There was a problem hiding this comment.
That's still an open question indeed, I've added it to the Todo's of this PR description already.
Member
There was a problem hiding this comment.
@AntoineRichard and me discussed it and we would use these functions in IsaacLab so this makes sense to make them pubic
|
|
||
| def parse_actuator_prim(prim) -> ParsedActuator | None: | ||
| """Parse a USD Actuator prim. Returns ``None`` if not a valid actuator.""" | ||
| if prim.GetTypeName() != "Actuator": |
Member
There was a problem hiding this comment.
We should probably also switch to using IsA rather than exact string matching, so we can have concrete derived types in USD
Comment on lines
+90
to
+106
| def determine_actuator_class(schemas: list[str]) -> type: | ||
| """Determine actuator class from inferred schemas.""" | ||
| has_delay = "DelayAPI" in schemas | ||
| has_pid = "PIDControllerAPI" in schemas | ||
| has_pd = "PDControllerAPI" in schemas | ||
| has_dc_motor = "DCMotorAPI" in schemas | ||
|
|
||
| if has_dc_motor and has_pd: | ||
| return ActuatorDCMotor | ||
| elif has_delay and has_pd: | ||
| return ActuatorDelayedPD | ||
| elif has_pid: | ||
| return ActuatorPID | ||
| elif has_pd: | ||
| return ActuatorPD | ||
| else: | ||
| return ActuatorPD |
Member
There was a problem hiding this comment.
This is a very restricted listing, while the schema design currently is much more flexible & lets end users "invent" new actuators by combining these API schemas.
How do we envision a user who invents the ElbowDrive (example from the SRD) to get that parsed through USD?
def Actuator "Elbow_Drive" (
prepend apiSchemas = ["DCMotorAPI", "PDControllerAPI", "DelayAPI"]
)
They would presumably also have a corresponding foo.ActuatorElbowDrive python class deriving newton.actuators.Actuator & expect/hope their delayed dc motor gets parsed into that class.
But I don't think this current parsing logic allows that, does it?
4 tasks
Member
Author
|
Closing in favor of #2449. |
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
5 participants
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.
Open questions
ParsedActuatorand the functionparse_actuator_prim?Summary by CodeRabbit
New Features
Documentation
Tests
Chores