Proposal of modularity#10
Conversation
📝 WalkthroughWalkthroughThe pull request restructures the actuator system from specialized classes (ActuatorPD, ActuatorPID, etc.) to a composable architecture where an Changes
Sequence Diagram(s)sequenceDiagram
participant Sim as Simulation
participant Actuator as Actuator<br/>(Composer)
participant Delay as Delay<br/>(Optional)
participant Controller as Controller
participant Clamp as Clamping<br/>(Optional)
participant Output as Force Output
Sim->>Actuator: step(sim_state, sim_control, states, dt)
alt Delay exists and is_ready()
Actuator->>Delay: get_delayed_targets(...)
Delay->>Actuator: return delayed_pos, delayed_vel, delayed_act
Actuator->>Controller: compute(delayed_pos, delayed_vel, ...)
else Delay exists but NOT ready
Actuator->>Delay: update_state(...)
Note over Actuator: Skip controller/clamp/output
else No Delay
Actuator->>Controller: compute(current_pos, current_vel, ...)
end
Controller->>Controller: force = kp*(target_pos - pos) + kd*(target_vel - vel)
Controller->>Actuator: return computed_forces
alt Clamping configured
Actuator->>Clamp: modify_forces(computed_forces, ...)
Clamp->>Clamp: apply saturation/lookup-table/DC-motor limits
Clamp->>Actuator: return applied_forces
else No Clamping
Actuator->>Actuator: applied_forces = computed_forces
end
Actuator->>Output: scatter-add(applied_forces, indices, joint_f)
alt Dual output enabled
Actuator->>Output: scatter-add(computed_forces, indices, joint_f_computed)
end
Actuator->>Controller: update_state(current_state, next_state)
Actuator->>Delay: update_state(target_pos, target_vel, ...)
Actuator->>Sim: force output updated
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes The changes introduce a comprehensive architectural refactor spanning multiple new modules with dense logic (Actuator composer, Controller base, five controller implementations, Delay with circular buffering, three Clamping variants), deletion of seven old actuator classes, significant USD parser rewrite using schema registry, and extensive test refactoring. While individual cohorts follow consistent patterns, the breadth of heterogeneous replacements and the density of new state-management logic demand careful cross-module verification. Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton_actuators/__init__.py (1)
6-25:⚠️ Potential issue | 🟡 MinorFix import sorting to resolve pipeline failure.
The CI reports
ruff check failed (I001): Import block is un-sorted or un-formatted. Imports need to be sorted alphabetically.Suggested fix
-from ._src.actuator import Actuator, ActuatorState +from ._src.actuator import Actuator, ActuatorState +from ._src.clamping import ( + Clamp, + Clamping, + DCMotorSaturation, + RemotizedClamp, +) from ._src.controllers import ( Controller, NetLSTMController, NetMLPController, PDController, PIDController, ) from ._src.delay import Delay -from ._src.clamping import ( - Clamp, - DCMotorSaturation, - Clamping, - RemotizedClamp, -) from ._src.usd_parser import ( ParsedActuator, parse_actuator_prim, ) from ._version import __version__Note: The exact order depends on your ruff/isort configuration. Run
ruff check --fixlocally to auto-format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/__init__.py` around lines 6 - 25, The import block in __init__.py is not alphabetically sorted per ruff/isort; reorder the import groups and the names within each from-import to be alphabetical (e.g., modules: from ._src.actuator, from ._src.clamping, from ._src.controllers, from ._src.delay, from ._src.usd_parser, then from ._version) and sort the identifiers inside each import (e.g., Controller, NetLSTMController, NetMLPController, PDController, PIDController; Clamp, Clamping, DCMotorSaturation, RemotizedClamp; ParsedActuator, parse_actuator_prim; Actuator, ActuatorState; __version__), then run ruff check --fix (or isort) to auto-apply formatting.
🧹 Nitpick comments (3)
newton_actuators/__init__.py (1)
38-43:Delayis miscategorized under the "Clamping" section.Per the
Clampingbase class docstring, Delay is passed separately to the Actuator, not as a Clamping component. Consider moving it to a separate category.Suggested organization
# Clamping "Clamping", "Clamp", "DCMotorSaturation", - "Delay", "RemotizedClamp", + # Delay + "Delay", # USD🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/__init__.py` around lines 38 - 43, The list in __init__.py incorrectly groups the Delay class under the Clamping section; remove "Delay" from the Clamping block and add it to a new or appropriate section for Actuator-level components (or a separate "Timing/Delay" category) so Delay is exported and documented separately from Clamping; update the surrounding comment header and any import/export order to reflect that Delay is associated with Actuator usage (referencing the Delay symbol and the Clamping base class docstring to guide placement).newton_actuators/_src/clamping/remotized_clamp.py (1)
36-49: Consider validating thatlookup_torquesvalues are non-negative.The kernel assumes
limitis positive for symmetric clamping[-limit, limit]. Iflookup_torquescontains negative values,wp.clamp(forces[i], -limit, limit)would have an inverted range, producing undefined behavior.Proposed validation in resolve_arguments or __init__
def __init__( self, lookup_angles: wp.array | tuple[float, ...] | list[float], lookup_torques: wp.array | tuple[float, ...] | list[float], ): ... if len(lookup_angles) != len(lookup_torques): raise ValueError(...) + if any(t < 0 for t in lookup_torques if not isinstance(lookup_torques, wp.array)): + raise ValueError("lookup_torques must contain non-negative values") self.lookup_size = len(lookup_angles)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/clamping/remotized_clamp.py` around lines 36 - 49, The kernel _remotized_clamp_kernel assumes the interpolated limit from lookup_torques is non-negative but negative entries would invert the clamp range; add validation of the lookup_torques array (e.g., in the class __init__ or the resolve_arguments function that prepares kernel args) to ensure all values are >= 0 and raise a clear exception (or log and clamp negatives to zero) if any negative values are found, and mention _interp_1d and lookup_torques in the error message so callers can locate and correct the data.newton_actuators/_src/controllers/pid.py (1)
108-113: Avoid storing pending state on the controller instance.
_next_integralis shared mutable scratch onPIDController, so a secondcompute()on the same instance can overwrite the first pending update beforeupdate_state()copies it. That makes state advancement depend on call ordering rather than thecurrent_state/next_stateobjects the API already exposes. Prefer writing directly intonext_state.integralor moving the scratch buffer into the state object.Also applies to: 138-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/controllers/pid.py` around lines 108 - 113, The PIDController currently stores shared mutable scratch _next_integral (set in set_indices), which can be overwritten across concurrent compute() calls; instead remove _next_integral and write directly into the provided next_state.integral (or move the scratch into the state object) wherever the controller prepares per-timestep updates (e.g., in set_indices and compute()); update code paths that reference _next_integral (including the logic between lines 138-166) to accept/write into next_state.integral and ensure update_state() consumes that per-instance next_state, eliminating the controller-level pending buffer.
🤖 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_actuators/_src/actuator.py`:
- Around line 4-14: The import block in newton_actuators/_src/actuator.py is not
sorted/formatted and imports an unused symbol; remove the unused "field" import
from dataclasses and reorder imports into standard groups (stdlib, third-party,
local) and alphabetically (e.g., from __future__, dataclasses, typing, then
numpy/warp, then local modules .controllers.base, .delay, .clamping.base), then
run the project's formatter/isort to ensure CI passes; keep the existing
"dataclass" and "Any" imports if they are used elsewhere in this module.
- Around line 17-29: The _scatter_add_kernel uses a non-atomic read-modify-write
(output[out_idx] = output[out_idx] + forces[i]) which can lose updates when
multiple threads target the same out_idx; replace that line with the Warp atomic
add primitive (e.g., call wp.atomic_add(output, out_idx, forces[i])) inside
_scatter_add_kernel so each thread updates output[out_idx] atomically and
prevents race conditions.
In `@newton_actuators/_src/clamping/dc_motor_saturation.py`:
- Around line 1-2: Update the SPDX copyright header in
newton_actuators/_src/clamping/dc_motor_saturation.py to match the project's
year convention (change "2026" to "2025") so it is consistent with other files
in the PR; modify the top-of-file comment lines containing
"SPDX-FileCopyrightText" and "SPDX-License-Identifier" accordingly.
In `@newton_actuators/_src/controllers/pid.py`:
- Around line 19-27: Warp kernels (_pid_force_kernel, _pd_force_kernel, and
_delay_buffer_state_kernel) declare concrete wp.array parameters (control_input,
constant_force) but compute() may pass None; fix by ensuring these parameters
are never None at kernel call sites: in the class constructor/compute()
materialize zero-filled wp.arrays for control_input and constant_force when None
is supplied, remove the optional | None type hints from the constructor and
kernel signatures (e.g., update compute(), __init__, and the kernel parameter
types for _pid_force_kernel), and update callers to always pass the zero arrays;
apply the same change for the corresponding names in pd.py (_pd_force_kernel and
its compute/constructor) and delay.py (_delay_buffer_state_kernel).
In `@newton_actuators/_src/usd_parser.py`:
- Around line 9-11: The import block in usd_parser.py is not sorted; reorder and
format the imports so they satisfy the linter (group standard library,
third‑party, then local imports) and alphabetize within groups — specifically
ensure the local imports for NetLSTMController, NetMLPController, PDController,
PIDController, Delay, Clamp, and DCMotorSaturation are sorted and formatted (or
run isort/black to auto-fix) so the import block passes CI.
---
Outside diff comments:
In `@newton_actuators/__init__.py`:
- Around line 6-25: The import block in __init__.py is not alphabetically sorted
per ruff/isort; reorder the import groups and the names within each from-import
to be alphabetical (e.g., modules: from ._src.actuator, from ._src.clamping,
from ._src.controllers, from ._src.delay, from ._src.usd_parser, then from
._version) and sort the identifiers inside each import (e.g., Controller,
NetLSTMController, NetMLPController, PDController, PIDController; Clamp,
Clamping, DCMotorSaturation, RemotizedClamp; ParsedActuator,
parse_actuator_prim; Actuator, ActuatorState; __version__), then run ruff check
--fix (or isort) to auto-apply formatting.
---
Nitpick comments:
In `@newton_actuators/__init__.py`:
- Around line 38-43: The list in __init__.py incorrectly groups the Delay class
under the Clamping section; remove "Delay" from the Clamping block and add it to
a new or appropriate section for Actuator-level components (or a separate
"Timing/Delay" category) so Delay is exported and documented separately from
Clamping; update the surrounding comment header and any import/export order to
reflect that Delay is associated with Actuator usage (referencing the Delay
symbol and the Clamping base class docstring to guide placement).
In `@newton_actuators/_src/clamping/remotized_clamp.py`:
- Around line 36-49: The kernel _remotized_clamp_kernel assumes the interpolated
limit from lookup_torques is non-negative but negative entries would invert the
clamp range; add validation of the lookup_torques array (e.g., in the class
__init__ or the resolve_arguments function that prepares kernel args) to ensure
all values are >= 0 and raise a clear exception (or log and clamp negatives to
zero) if any negative values are found, and mention _interp_1d and
lookup_torques in the error message so callers can locate and correct the data.
In `@newton_actuators/_src/controllers/pid.py`:
- Around line 108-113: The PIDController currently stores shared mutable scratch
_next_integral (set in set_indices), which can be overwritten across concurrent
compute() calls; instead remove _next_integral and write directly into the
provided next_state.integral (or move the scratch into the state object)
wherever the controller prepares per-timestep updates (e.g., in set_indices and
compute()); update code paths that reference _next_integral (including the logic
between lines 138-166) to accept/write into next_state.integral and ensure
update_state() consumes that per-instance next_state, eliminating the
controller-level pending buffer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f6b8ea3-f0c9-4692-ac7f-19df01ff5871
📒 Files selected for processing (27)
README.mdnewton_actuators/__init__.pynewton_actuators/_src/actuator.pynewton_actuators/_src/actuators/__init__.pynewton_actuators/_src/actuators/base.pynewton_actuators/_src/actuators/dc_motor.pynewton_actuators/_src/actuators/delayed_pd.pynewton_actuators/_src/actuators/net_lstm.pynewton_actuators/_src/actuators/net_mlp.pynewton_actuators/_src/actuators/pd.pynewton_actuators/_src/actuators/pid.pynewton_actuators/_src/actuators/remotized_pd.pynewton_actuators/_src/clamping/__init__.pynewton_actuators/_src/clamping/base.pynewton_actuators/_src/clamping/clamp.pynewton_actuators/_src/clamping/dc_motor_saturation.pynewton_actuators/_src/clamping/remotized_clamp.pynewton_actuators/_src/controllers/__init__.pynewton_actuators/_src/controllers/base.pynewton_actuators/_src/controllers/net_lstm.pynewton_actuators/_src/controllers/net_mlp.pynewton_actuators/_src/controllers/pd.pynewton_actuators/_src/controllers/pid.pynewton_actuators/_src/delay.pynewton_actuators/_src/kernels.pynewton_actuators/_src/usd_parser.pynewton_actuators/tests/test_actuators.py
💤 Files with no reviewable changes (10)
- newton_actuators/_src/actuators/init.py
- newton_actuators/_src/actuators/dc_motor.py
- newton_actuators/_src/kernels.py
- newton_actuators/_src/actuators/base.py
- newton_actuators/_src/actuators/pd.py
- newton_actuators/_src/actuators/net_lstm.py
- newton_actuators/_src/actuators/net_mlp.py
- newton_actuators/_src/actuators/delayed_pd.py
- newton_actuators/_src/actuators/remotized_pd.py
- newton_actuators/_src/actuators/pid.py
| # TODO: replace with a Transmission class that applies gear ratios / linkage | ||
| # transforms before accumulating into the output array. | ||
| @wp.kernel | ||
| def _scatter_add_kernel( | ||
| forces: wp.array(dtype=float), | ||
| output_indices: wp.array(dtype=wp.uint32), | ||
| output: wp.array(dtype=float), | ||
| ): | ||
| """Accumulate forces into output at specified indices.""" | ||
| i = wp.tid() | ||
| out_idx = output_indices[i] | ||
| output[out_idx] = output[out_idx] + forces[i] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if output_indices are expected to be unique or can have duplicates
rg -n "output_indices" --type py -C 3
# Check if there are any tests with duplicate output indices
rg -n "output_indices.*=.*\[" --type pyRepository: newton-physics/newton-actuators
Length of output: 22319
🏁 Script executed:
# Check for any documentation about multi-output or shared outputs
rg -i "multi.*output|shared.*output|duplicate.*index" --type py --type md
# Look for comments or issues related to the kernel or race conditions
rg -i "race|atomic|concurrent|thread.*safety" newton_actuators/ --type py
# Check if there's any usage where multiple Actuators write to the same output array
rg -n "output_indices.*reshape|output_indices.*flatten" --type pyRepository: newton-physics/newton-actuators
Length of output: 266
🏁 Script executed:
# Look at the multi-output test to understand the intended behavior
rg -n "test_multi_step_different_outputs" --type py -A 30
# Check the Actuator class initialization to understand multi-output shape
sed -n '70,130p' newton_actuators/_src/actuator.py
# Search for any reshaping or flattening of output_indices
rg -n "reshape|flatten" newton_actuators/_src/actuator.py
# Check if output arrays are pre-initialized with zeros
rg -n "output.*=.*0|wp.zeros" newton_actuators/_src/ --type py -B 2 -A 2Repository: newton-physics/newton-actuators
Length of output: 7413
🏁 Script executed:
# Check how output array is used - does it get reset between calls?
rg -n "control_output_attr|joint_f" newton_actuators/_src/actuator.py -B 2 -A 2
# Look at the step() function to see the full kernel launch context
sed -n '200,270p' newton_actuators/_src/actuator.py
# Check if there's any mention of concurrent Actuators or shared outputs
rg -n "concurrent|parallel|multi.*actuator" newton_actuators/ --type py --type mdRepository: newton-physics/newton-actuators
Length of output: 4032
🏁 Script executed:
# Check if step() is designed to be called for multiple actuators on the same output
rg -n "step.*dim|for.*actuator.*step" newton_actuators/ --type py -B 2 -A 2
# Look at example usage or documentation showing multiple actuators
fd -e md -e rst . | head -5 | xargs cat 2>/dev/null | head -100
# Check if there's a test that creates multiple Actuators and calls step() sequentially
rg -n "actuator.*step|step.*output" newton_actuators/tests/ --type py -B 1 -A 1 | head -50Repository: newton-physics/newton-actuators
Length of output: 10198
Use atomic add to prevent race conditions with duplicate output indices.
The kernel performs output[out_idx] = output[out_idx] + forces[i] using a non-atomic read-modify-write. If multiple threads write to the same output_idx (whether from duplicate indices within a single Actuator or multiple Actuators orchestrated to the same DOF), this creates a data race with lost updates. Replace with atomic addition:
Proposed fix
`@wp.kernel`
def _scatter_add_kernel(
forces: wp.array(dtype=float),
output_indices: wp.array(dtype=wp.uint32),
output: wp.array(dtype=float),
):
"""Accumulate forces into output at specified indices."""
i = wp.tid()
out_idx = output_indices[i]
- output[out_idx] = output[out_idx] + forces[i]
+ wp.atomic_add(output, out_idx, forces[i])📝 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.
| # TODO: replace with a Transmission class that applies gear ratios / linkage | |
| # transforms before accumulating into the output array. | |
| @wp.kernel | |
| def _scatter_add_kernel( | |
| forces: wp.array(dtype=float), | |
| output_indices: wp.array(dtype=wp.uint32), | |
| output: wp.array(dtype=float), | |
| ): | |
| """Accumulate forces into output at specified indices.""" | |
| i = wp.tid() | |
| out_idx = output_indices[i] | |
| output[out_idx] = output[out_idx] + forces[i] | |
| # TODO: replace with a Transmission class that applies gear ratios / linkage | |
| # transforms before accumulating into the output array. | |
| `@wp.kernel` | |
| def _scatter_add_kernel( | |
| forces: wp.array(dtype=float), | |
| output_indices: wp.array(dtype=wp.uint32), | |
| output: wp.array(dtype=float), | |
| ): | |
| """Accumulate forces into output at specified indices.""" | |
| i = wp.tid() | |
| out_idx = output_indices[i] | |
| wp.atomic_add(output, out_idx, forces[i]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/actuator.py` around lines 17 - 29, The
_scatter_add_kernel uses a non-atomic read-modify-write (output[out_idx] =
output[out_idx] + forces[i]) which can lose updates when multiple threads target
the same out_idx; replace that line with the Warp atomic add primitive (e.g.,
call wp.atomic_add(output, out_idx, forces[i])) inside _scatter_add_kernel so
each thread updates output[out_idx] atomically and prevents race conditions.
| # SPDX-FileCopyrightText: Copyright (c) 2026 The Newton Developers | ||
| # SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Copyright year inconsistency.
This file uses 2026 while other files in this PR use 2025. Consider aligning for consistency.
Suggested fix
-# SPDX-FileCopyrightText: Copyright (c) 2026 The Newton Developers
+# SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers📝 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.
| # SPDX-FileCopyrightText: Copyright (c) 2026 The Newton Developers | |
| # SPDX-License-Identifier: Apache-2.0 | |
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers | |
| # SPDX-License-Identifier: Apache-2.0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/clamping/dc_motor_saturation.py` around lines 1 - 2,
Update the SPDX copyright header in
newton_actuators/_src/clamping/dc_motor_saturation.py to match the project's
year convention (change "2026" to "2025") so it is consistent with other files
in the PR; modify the top-of-file comment lines containing
"SPDX-FileCopyrightText" and "SPDX-License-Identifier" accordingly.
| control_input: wp.array(dtype=float), | ||
| state_indices: wp.array(dtype=wp.uint32), | ||
| target_indices: wp.array(dtype=wp.uint32), | ||
| force_indices: wp.array(dtype=wp.uint32), | ||
| kp: wp.array(dtype=float), | ||
| ki: wp.array(dtype=float), | ||
| kd: wp.array(dtype=float), | ||
| integral_max: wp.array(dtype=float), | ||
| constant_force: wp.array(dtype=float), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "pid.py" | head -20Repository: newton-physics/newton-actuators
Length of output: 117
🏁 Script executed:
cat -n ./newton_actuators/_src/controllers/pid.pyRepository: newton-physics/newton-actuators
Length of output: 6226
🏁 Script executed:
# Check if there are other Warp kernels in the codebase that handle optional/None parameters
rg -t py "@wp\.kernel" -A 30 | head -100Repository: newton-physics/newton-actuators
Length of output: 6986
🌐 Web query:
Warp kernel optional parameters None typing.Any
💡 Result:
In NVIDIA Warp (@wp.kernel), kernel arguments must be statically typed, because kernels are compiled to native CPU/CUDA code. That means you can’t have “optional” kernel parameters in the Python sense (e.g., an argument whose type might be None at runtime). If you need “optional behavior”, you typically pass an extra flag/value (e.g., use_x: int) and branch inside the kernel, or write/overload separate kernels with different signatures. [1]
typing.Any in a kernel signature does not mean “any runtime value (including None)”; it’s a generic type placeholder that Warp will instantiate into concrete, compiled kernel variants based on the types you pass at launch time. [2]
Sources:
[1] Warp Basics (static typing / kernel params must be annotated) (nvidia.github.io)
[2] Warp Generics (typing.Any generic kernels) (nvidia.github.io)
Citations:
- 1: https://nvidia.github.io/warp/user_guide/basics.html
- 2: https://nvidia.github.io/warp/user_guide/generics.html
🏁 Script executed:
# Check if there are tests that verify the None case for control_input or constant_force
find . -type f -name "*.py" | xargs rg -l "PIDController|pid" | grep -i testRepository: newton-physics/newton-actuators
Length of output: 117
🏁 Script executed:
cat -n ./newton_actuators/tests/test_actuators.py | head -200Repository: newton-physics/newton-actuators
Length of output: 8161
🏁 Script executed:
cat -n ./newton_actuators/tests/test_actuators.py | sed -n '200,400p'Repository: newton-physics/newton-actuators
Length of output: 9091
🏁 Script executed:
# Search for tests that use None for constant_force or control_input
rg -n "constant_force|control_input" ./newton_actuators/tests/test_actuators.py | head -20Repository: newton-physics/newton-actuators
Length of output: 136
🏁 Script executed:
# Search for where PIDController is instantiated to see if None is ever passed
rg -B 3 -A 3 "PIDController(" ./newton_actuators/Repository: newton-physics/newton-actuators
Length of output: 153
🏁 Script executed:
# Search for where PIDController is instantiated to see if None is ever passed
rg "PIDController\(" ./newton_actuators/Repository: newton-physics/newton-actuators
Length of output: 393
🏁 Script executed:
# Check all PIDController instantiations in tests
rg -B 5 -A 10 "controller=PIDController" ./newton_actuators/tests/test_actuators.pyRepository: newton-physics/newton-actuators
Length of output: 1992
🏁 Script executed:
# Check if there's documentation or examples in the codebase about handling optional arrays
find . -type f -name "*.py" -o -name "*.md" | xargs rg -l "wp.launch" | head -5Repository: newton-physics/newton-actuators
Length of output: 286
🏁 Script executed:
# Check how pd.py handles compute to see if it has similar pattern
cat -n ./newton_actuators/_src/controllers/pd.py | head -150Repository: newton-physics/newton-actuators
Length of output: 4020
🏁 Script executed:
# Check the base Controller interface to understand the contract
find . -type f -name "base.py" -path "*/controllers/*"Repository: newton-physics/newton-actuators
Length of output: 118
🏁 Script executed:
cat -n ./newton_actuators/_src/controllers/base.pyRepository: newton-physics/newton-actuators
Length of output: 4919
🏁 Script executed:
# Let's check if there are any issues reported or if this pattern works in practice
# by looking for any warp.launch calls that pass None
rg -B 5 -A 5 "wp.launch" ./newton_actuators/_src/delay.py | head -40Repository: newton-physics/newton-actuators
Length of output: 375
Fix Warp kernel parameter types to handle optional arrays.
_pid_force_kernel declares control_input and constant_force as concrete wp.array(dtype=float) parameters (lines 19, 27) but compute() passes both as potentially None (lines 146, 154). Warp's kernels require static typing for compilation; they cannot accept None in concrete array slots. The runtime branching on these parameters (if constant_force: at line 46, if control_input: at line 50) will fail or behave unpredictably when None is passed.
Either:
- Always materialize zero-filled arrays and remove the
| Nonetype hints from the constructor and compute signature, or - Use
typing.Anyfor these parameters and split into explicit kernel variants for the None/not-None cases.
Same issue in _pd_force_kernel (pd.py lines 23, 36–41) and _delay_buffer_state_kernel (delay.py).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/controllers/pid.py` around lines 19 - 27, Warp kernels
(_pid_force_kernel, _pd_force_kernel, and _delay_buffer_state_kernel) declare
concrete wp.array parameters (control_input, constant_force) but compute() may
pass None; fix by ensuring these parameters are never None at kernel call sites:
in the class constructor/compute() materialize zero-filled wp.arrays for
control_input and constant_force when None is supplied, remove the optional |
None type hints from the constructor and kernel signatures (e.g., update
compute(), __init__, and the kernel parameter types for _pid_force_kernel), and
update callers to always pass the zero arrays; apply the same change for the
corresponding names in pd.py (_pd_force_kernel and its compute/constructor) and
delay.py (_delay_buffer_state_kernel).
| """Return True if all components can be captured in a CUDA graph.""" | ||
| return self.controller.is_graphable() and all(c.is_graphable() for c in self.clamping) | ||
|
|
||
| def has_transmission(self) -> bool: |
There was a problem hiding this comment.
This we will remove still right?
There was a problem hiding this comment.
I was thinking to add an interface already now, but just returning False for now. We can also fully remove it.
| controller_kwargs: dict[str, Any] = field(default_factory=dict) | ||
| component_specs: list[tuple[type, dict[str, Any]]] = field(default_factory=list) | ||
| target_paths: list[str] = field(default_factory=list) | ||
| transmission: list[float] | None = None |
There was a problem hiding this comment.
This we'll remove still right?
There was a problem hiding this comment.
I agree, it may be best to remove transmission until it is actually supported
| schemas = infer_schemas_from_prim(prim) | ||
|
|
||
| schemas = get_schemas_from_prim(prim) | ||
| transmission = get_attribute(prim, "newton:actuator:transmission") |
There was a problem hiding this comment.
This we would remove still right?
| component_specs=component_specs, | ||
| target_paths=target_paths, | ||
| kwargs=extract_kwargs_from_prim(prim, schemas), | ||
| transmission=list(transmission) if transmission else None, |
There was a problem hiding this comment.
This we would remove still right?
| GPU-accelerated actuator library for physics simulations. | ||
|
|
||
| This library provides a collection of actuator implementations that integrate with physics simulation pipelines. Actuators read from simulation state arrays and write computed forces/torques back to control arrays. | ||
| This library provides composable actuator implementations that integrate with |
There was a problem hiding this comment.
How about this instead which adds a bit more info of what we want to achieve?
This library provides composable actuator implementations that read physics simulation state, compute actuator forces, and write the forces back to control arrays for application to the simulation. The simulator does not need to be part of Newton: the library is designed to be reusable anywhere the caller can provide state arrays and consume forces. Each actuator instance is vectorized: a single actuator object operates on a batch of indices in global state and control arrays, allowing efficient integration into RL workflows. The goal is to provide canonical actuator models with support for differentiability and graphable execution where the underlying controller implementation supports it. The library is designed to be easy for users to customize and extend for their specific actuator models.
There was a problem hiding this comment.
Whats also missing here or in the arch is the fact that the actuators are vectorized no?
There was a problem hiding this comment.
Added a sentence now here about the vectorization.
| composes a controller with an optional delay and zero or more clamping objects. | ||
| The `step()` method runs: | ||
|
|
||
| 1. **Delay** — read delayed targets from buffer (skipped if no delay or buffer still filling) |
There was a problem hiding this comment.
The force is zero if still filling?
|
|
||
| 1. **Delay** — read delayed targets from buffer (skipped if no delay or buffer still filling) | ||
| 2. **Controller** — compute raw forces | ||
| 3. **Clamping** — bound forces (e.g. `Clamp`, `DCMotorSaturation`) |
There was a problem hiding this comment.
I would just repeat "Clamp raw forces (e.g....)"
| 1. **Delay** — read delayed targets from buffer (skipped if no delay or buffer still filling) | ||
| 2. **Controller** — compute raw forces | ||
| 3. **Clamping** — bound forces (e.g. `Clamp`, `DCMotorSaturation`) | ||
| 4. **Scatter-add** — accumulate forces into the output array |
There was a problem hiding this comment.
Scatter we don't need to do w/o transmission right?
There was a problem hiding this comment.
we need, to write into the right places in the force array
though I dont think we need output and input indices separately, considering that no transmission, so I would just make indices
There was a problem hiding this comment.
Ah got it going from actuator space to dof space, thanks.
Simplifying indexing until we need a more advanced transmission setup makes sense.
| - `actuator.is_graphable()` — True if `step()` can be captured in a CUDA graph | ||
| - `actuator.state()` — return a new `ActuatorState` (None if stateless) | ||
| - `actuator.step(sim_state, sim_control, current_state, next_state, dt)` — one control step | ||
| - `state.reset()` — zero all internal buffers without reallocating |
There was a problem hiding this comment.
Indexing TBD and this should be api links when we move to newton repo.
| 1. **Create an actuator** by composing a controller with clamping | ||
| 2. **Check statefulness**: call `actuator.is_stateful()` | ||
| 3. **Initialize states**: for stateful actuators, create double-buffered states with `actuator.state()` | ||
| 4. **Simulation loop**: call `actuator.step()` each timestep |
There was a problem hiding this comment.
Could comment on substeps here too? But maybe TMI?
| "Clamp", | ||
| "DCMotorSaturation", | ||
| "Clamping", | ||
| "RemotizedClamp", |
There was a problem hiding this comment.
There was some discussion in the doc - but is "RemotizedClamp" a well-known term? Would "PositionBasedClamping" be more clear?
| act_input: Feedforward control input (may be None). | ||
| input_indices: Indices into positions/velocities. | ||
| target_indices: Indices into target arrays. | ||
| forces: Scratch buffer to write forces to. Shape (N,). | ||
| force_indices: Indices into forces buffer (typically sequential). | ||
| num_actuators: Number of actuators N. | ||
| state: Controller state (None if stateless). | ||
| dt: Timestep in seconds. |
There was a problem hiding this comment.
Couple of comments/questions:
- Would
feedforward_controlbe a better name foract_input? I wasn't sure whatact_inputwas before reading the docstring. - Similar to what is usually done in C, did we want to put all outputs at the end of the function signature, and give them an ending like
forces_outandstate_outso that there is a clear separation of what is input and what is output?
There was a problem hiding this comment.
Also, in the base class, should we have one additional "other_inputs" kind of argument, to cover when there are actuators which take inputs we haven't thought of? I am thinking of controllers which take gains as real-time inputs rather than assuming they are fixed parameters.
| def is_graphable(self) -> bool: | ||
| """Return True if compute() can be captured in a CUDA graph.""" | ||
| return True |
There was a problem hiding this comment.
Should this be NotImplementedError for the base class?
There was a problem hiding this comment.
I think keeping True makes more sense because in general we are focusing on the graphable implementations. The ones which are not are an exception, they are using pytorch and exist only for the reason to be compatible with the IsaacLab policies. I think in the future the plan is to replace with warpnn based and then this is_graphable would not be relevant at all.
| The network must be callable as: | ||
| torques, (h_new, c_new) = network(input, (h, c)) |
There was a problem hiding this comment.
Do h and c have more meaningful names we could add?
| # TODO: replace with a Transmission class that applies gear ratios / linkage | ||
| # transforms before accumulating into the output array. |
There was a problem hiding this comment.
Question about this:
Our current design assumes that the gear-ratio is "baked in" to the controller. In that case, the transmission should not include gear-ratios, i.e. not what this comment would indicate? The transmission will only be used to model things like cabling, or massless linkages.
There was a problem hiding this comment.
in general yes, transmission is intended for things like that.
That being said, our design does not exclude for the user a possibility to add a gear box as a transmission mechanism and provide all parameters/state and targets in the motor space.
| controller_kwargs: dict[str, Any] = field(default_factory=dict) | ||
| component_specs: list[tuple[type, dict[str, Any]]] = field(default_factory=list) | ||
| target_paths: list[str] = field(default_factory=list) | ||
| transmission: list[float] | None = None |
There was a problem hiding this comment.
I agree, it may be best to remove transmission until it is actually supported
| "Clamping", | ||
| "Clamp", | ||
| "DCMotorSaturation", | ||
| "Delay", |
There was a problem hiding this comment.
I guess this shouldn't go under the "clamping" header :)
| | Controller | Description | Stateful | | ||
| |---|---|---| | ||
| | `PDController` | Proportional-derivative controller | No | | ||
| | `PIDController` | PID controller with anti-windup integral | Yes | |
There was a problem hiding this comment.
I think it is best to say something like "with integral clamping" to be explicit. There are other forms of anti-windup, such as "exponential forgetting" which are not clamping based.
| class ParsedActuator: | ||
| """Result of parsing a USD actuator prim. |
There was a problem hiding this comment.
I might be missing it - but is there a supported way to convert ParsedActuator into an Actuator?
There was a problem hiding this comment.
it is responsibility of the user code to correctly aggregate information and construct indices arrays and instantiate the actual Actuators. Please check how it is done in newton when the prev version of this library was integrated
|
I think we should keep the Newton naming principle here and prefer prefix-first names. In Newton we generally want the category/type first and the specialization second, e.g.
Applied to the names introduced in this PR, that would suggest:
I’m not attached to every individual replacement above, but I do think we should be consistent about the direction: prefix-first rather than suffix-first. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton_actuators/tests/test_actuators.py (1)
1-1396:⚠️ Potential issue | 🟡 MinorRun
ruff formaton this file before merge.CI is already red with “file would be reformatted”, so this test module needs a formatter pass as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/tests/test_actuators.py` around lines 1 - 1396, The file fails ruff formatting; run the formatter (e.g., ruff format) over this test module (the one containing classes like TestPDWithClampingMaxForce, TestPDWithDelay, TestPIDWithClampingMaxForce, TestPDWithDCMotor, TestPDWithClampingPositionBased, TestComposition, and TestControllerNetMLP/LSTM) and commit the reformatted result so CI no longer reports "file would be reformatted".
♻️ Duplicate comments (4)
newton_actuators/_src/controllers/controller_pid.py (1)
19-27:⚠️ Potential issue | 🟠 MajorDon't launch
_pid_force_kernelwithNonearray inputs.
compute()can still passact_input=Noneandself.constant_force=None, but the kernel signature requires concretewp.array(...)arguments. The runtimeif control_input/if constant_forcechecks do not fix that mismatch, so this path is still unsafe until you materialize zero arrays or split the kernel variants.Also applies to: 45-53, 85-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/controllers/controller_pid.py` around lines 19 - 27, The kernel _pid_force_kernel must never be launched with Python None for array parameters (e.g., control_input, constant_force); update compute() to ensure all kernel arguments are concrete wp.array instances by either (a) creating zero/empty wp.array placeholders for any None inputs (matching expected dtype and length) before calling _pid_force_kernel, or (b) split into two kernel variants and call the correct one when control_input or constant_force is None; adjust the call sites in compute() and any places that pass state_indices/target_indices/force_indices so they always supply valid wp.array objects rather than None.newton_actuators/_src/controllers/controller_pd.py (1)
17-24:⚠️ Potential issue | 🟠 MajorDon't pass optional arrays into
_pd_force_kernel.
control_inputandconstant_forceare declared as concretewp.array(...)kernel params, butcompute()can still launch withact_input=Noneandself.constant_force=None. That keeps the old Warp typing issue alive here, and the in-kernel truthiness checks don't make the launch safe. Please materialize zero-filled arrays before launch, or split the kernel variants.Also applies to: 35-41, 79-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/controllers/controller_pd.py` around lines 17 - 24, The kernel _pd_force_kernel currently declares control_input and constant_force as concrete wp.array kernel params but compute() may call the kernel with act_input=None or self.constant_force=None which is unsafe; before launching _pd_force_kernel (and the other kernel variants referenced around lines 35-41 and 79-111), ensure you materialize zero-filled wp.array instances when act_input or constant_force is None (e.g., create arrays of the correct dtype/shape filled with zeros matching state_indices/target_indices length) or alternatively provide a separate kernel overload that omits those parameters; update the caller in compute() to replace None with those zero arrays so the kernel never receives None for control_input/constant_force.newton_actuators/__init__.py (1)
27-48: 🛠️ Refactor suggestion | 🟠 MajorSettle the public naming scheme before exporting it.
__all__now bakes suffix-first names into the package API. Since the review thread already points toward prefix-first naming, merging these exports as-is turns that rename into a breaking change later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/__init__.py` around lines 27 - 48, The package exports in __all__ use suffix-first names (ControllerPD, ControllerPID, ControllerNetMLP, ControllerNetLSTM, ClampingMaxForce, ClampingPositionBased, ClampingVelocityBased) which conflicts with the agreed prefix-first naming; update __all__ to export the prefix-first variants (e.g. PDController, PIDController, NetMLPController, NetLSTMController, MaxForceClamping or PositionBasedClamping, VelocityBasedClamping) or remove the suffix-first entries until the rename is completed across the codebase; ensure the exported names in __all__ match the actual class/function identifiers (Actuator, StateActuator, Controller, Delay, Clamping, ParsedActuator, parse_actuator_prim) so importing modules won’t break.newton_actuators/_src/actuator.py (1)
17-42:⚠️ Potential issue | 🔴 CriticalUse atomic adds in both scatter kernels.
Both kernels perform non-atomic read/modify/write on shared output arrays. If two entries target the same DOF, one update can be lost. This was already raised earlier and still looks unresolved.
Suggested fix
def _scatter_add_kernel( forces: wp.array(dtype=float), indices: wp.array(dtype=wp.uint32), output: wp.array(dtype=float), ): """Scatter-add forces into output at specified indices.""" i = wp.tid() idx = indices[i] - output[idx] = output[idx] + forces[i] + wp.atomic_add(output, idx, forces[i]) @@ def _scatter_add_dual_kernel( applied_forces: wp.array(dtype=float), computed_forces: wp.array(dtype=float), indices: wp.array(dtype=wp.uint32), applied_output: wp.array(dtype=float), computed_output: wp.array(dtype=float), ): """Scatter-add both applied and computed forces in one pass.""" i = wp.tid() idx = indices[i] - applied_output[idx] = applied_output[idx] + applied_forces[i] - computed_output[idx] = computed_output[idx] + computed_forces[i] + wp.atomic_add(applied_output, idx, applied_forces[i]) + wp.atomic_add(computed_output, idx, computed_forces[i])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuator.py` around lines 17 - 42, The scatter kernels _scatter_add_kernel and _scatter_add_dual_kernel perform non-atomic read/modify/write on shared output arrays and must use atomic additions to avoid lost updates when multiple threads target the same index; replace the direct updates (e.g., output[idx] = output[idx] + forces[i] and applied_output[idx] = applied_output[idx] + applied_forces[i], computed_output[idx] = computed_output[idx] + computed_forces[i]) with the Warp atomic add primitive (e.g., wp.atomic.add) so each thread performs an atomic addition into output, applied_output, and computed_output respectively.
🤖 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_actuators/_src/actuator.py`:
- Around line 180-187: The step method currently has an optional dt parameter
which is actually required by downstream callers (compute() and
delay.update_state()), so either remove the default to make dt a required
positional/keyword argument on Actuator.step or validate it at the top of step
and raise a clear exception; update the signature on def step(..., dt: float) ->
None to drop the "= None" or add an early check like "if dt is None: raise
ValueError('dt is required')" before any calls to compute() or
delay.update_state() so no None is propagated into time-based math (refer to
step, compute, and delay.update_state in your changes).
- Around line 206-249: If the actuator is stateful (self.is_stateful() true),
ensure step() checks that both current_act_state and next_act_state are present
and raise a clear API error immediately (e.g., ValueError/RuntimeError) instead
of proceeding; add the guard at the start of the section that currently sets
has_states and before calling delay.is_ready() or controller.compute(), so
delay.is_ready(delay_state) and self.controller.compute(...) never run when
current_act_state or next_act_state is missing.
In `@newton_actuators/_src/clamping/base.py`:
- Around line 50-74: The base class modify_forces method currently is a silent
no-op; change it to fail fast by raising a clear error (e.g.,
NotImplementedError) inside modify_forces so subclasses must implement it:
locate the modify_forces method in newton_actuators._src.clamping.base (the base
clamping class) and replace the empty implementation with a raised exception
that includes a descriptive message referencing modify_forces and the clamping
class to aid debugging.
In `@newton_actuators/_src/clamping/clamping_max_force.py`:
- Around line 29-39: Ensure negative per-actuator limits are rejected by
validating max_force in both resolve_arguments and __init__: in
resolve_arguments(cls, args) check args.get("max_force", math.inf) for any
negative entries and raise a ValueError if so (include the offending value in
the message); in __init__(self, max_force: wp.array) validate the provided
wp.array has all elements >= 0 (e.g., use a vectorized test or iterate to detect
any element < 0) and raise ValueError if any are negative; keep the attribute
name max_force and preserve the existing behavior when values are non-negative.
In `@newton_actuators/_src/clamping/clamping_position_based.py`:
- Around line 1-133: This file fails ruff formatting; run the project's
auto-formatter (ruff format) on
newton_actuators/_src/clamping/clamping_position_based.py to fix whitespace,
line breaks, and import formatting issues; after reformatting, ensure the module
still compiles and that symbols like _interp_1d, _remotized_clamp_kernel, and
class ClampingPositionBased (resolve_arguments, set_device, modify_forces)
remain unchanged except for formatting edits, then update the PR.
- Around line 97-110: The constructor currently keeps pre-built wp.array objects
in self.lookup_angles/self.lookup_torques which prevents set_device() from
migrating them to the target device; update set_device() to detect when
self.lookup_angles or self.lookup_torques are wp.array instances that live on a
different device and re-create or copy them onto the provided device (so
_remotized_clamp_kernel never receives mixed-device inputs). Specifically, in
set_device() check the device of self.lookup_angles and self.lookup_torques and,
if they exist but are not on the target device, migrate them to the target
device (using Warp's array/copy mechanism) before returning.
In `@newton_actuators/_src/clamping/clamping_velocity_based.py`:
- Around line 53-61: In resolve_arguments of ClampingVelocityBased, validate
that the provided "velocity_limit" is strictly greater than 0 (for scalars) or
every element is > 0 (for sequences/arrays) and raise a ValueError if any value
is <= 0 to prevent division-by-zero/inverted behavior; update both the
scalar/sequence overloads (the resolve_arguments implementation and the similar
block referenced at lines ~63-78) to perform this positive-check before
returning the dict and keep returning "saturation_effort", "velocity_limit", and
"max_force" as before.
In `@newton_actuators/_src/controllers/controller_net_lstm.py`:
- Around line 147-151: The code currently chooses torch_target_idx by object
identity (comparing target_indices is self._warp_sequential_indices) which fails
when an equivalent array is passed; replace this with logic that derives the
torch index tensor from target_indices itself (or uses an explicit flag).
Concretely, add or reuse a helper like _torch_indices_for(target_indices) that
returns the correct tensor (e.g., by comparing contents with
np.array_equal/torch.equal or by constructing indices from target_indices
values) and use torch_target_idx = self._torch_indices_for(target_indices)
instead of the identity check between _torch_sequential_indices and
_warp_sequential_indices; alternatively accept an explicit parameter (e.g.,
targets_are_sequential) and branch on that rather than object identity.
In `@newton_actuators/_src/controllers/controller_net_mlp.py`:
- Around line 135-139: Replace the identity check that picks torch_target_idx
(currently using "target_indices is self._warp_sequential_indices") with a
stable content-based approach: convert incoming target_indices to a torch tensor
(e.g., torch.as_tensor(target_indices)) or accept an explicit indexing-mode flag
from the caller, then select between self._torch_sequential_indices and
self._torch_input_indices based on the content/flag. Update the logic around
torch_target_idx, target_indices, self._warp_sequential_indices,
self._torch_sequential_indices and self._torch_input_indices so indexing no
longer relies on object identity.
In `@newton_actuators/_src/usd_parser.py`:
- Around line 136-154: The parser currently accepts an applied schema even when
none of its required attributes are authored; to fix this, after collecting
kwargs from entry.param_map but before treating the schema as present, invoke
entry.validate(kwargs) unconditionally (or check explicitly if no params were
authored and then call validate) and propagate/raise any validation error with
context (include prim path/name and entry.component_class.__name__) so the
failure is tied to the prim/schema; only when validate succeeds should you set
controller_class/controller_kwargs or append to component_specs (using the
existing symbols entry.param_map, get_attribute, entry.validate,
entry.is_controller, controller_class, controller_kwargs, component_specs, and
entry.component_class).
In `@README.md`:
- Around line 24-32: The fenced code block showing the Actuator tree (lines
containing "Actuator", "Controller", "Delay", "Clamping", "ClampingMaxForce",
"ClampingVelocityBased", "ClampingPositionBased") should include a language tag
to satisfy markdownlint; update the triple-backtick fence to use a language like
"text" (or another suitable language) immediately after the opening ``` so the
block is fenced as ```text.
- Around line 214-216: The example uses the same buffer for both current and
next state when calling actuator.step, which breaks the double-buffered pattern;
update the docs to call actuator.state() to create two buffers (e.g., state and
next_state), pass state as the current state and next_state as the next state to
actuator.step(sim_state, sim_control, state, next_state, dt=0.01), then after
each step swap or reassign so the next_state becomes the current for the
following iteration; reference the actuator.state() factory and
actuator.step(sim_state, sim_control, current_state, next_state, dt) when
describing the pattern.
---
Outside diff comments:
In `@newton_actuators/tests/test_actuators.py`:
- Around line 1-1396: The file fails ruff formatting; run the formatter (e.g.,
ruff format) over this test module (the one containing classes like
TestPDWithClampingMaxForce, TestPDWithDelay, TestPIDWithClampingMaxForce,
TestPDWithDCMotor, TestPDWithClampingPositionBased, TestComposition, and
TestControllerNetMLP/LSTM) and commit the reformatted result so CI no longer
reports "file would be reformatted".
---
Duplicate comments:
In `@newton_actuators/__init__.py`:
- Around line 27-48: The package exports in __all__ use suffix-first names
(ControllerPD, ControllerPID, ControllerNetMLP, ControllerNetLSTM,
ClampingMaxForce, ClampingPositionBased, ClampingVelocityBased) which conflicts
with the agreed prefix-first naming; update __all__ to export the prefix-first
variants (e.g. PDController, PIDController, NetMLPController, NetLSTMController,
MaxForceClamping or PositionBasedClamping, VelocityBasedClamping) or remove the
suffix-first entries until the rename is completed across the codebase; ensure
the exported names in __all__ match the actual class/function identifiers
(Actuator, StateActuator, Controller, Delay, Clamping, ParsedActuator,
parse_actuator_prim) so importing modules won’t break.
In `@newton_actuators/_src/actuator.py`:
- Around line 17-42: The scatter kernels _scatter_add_kernel and
_scatter_add_dual_kernel perform non-atomic read/modify/write on shared output
arrays and must use atomic additions to avoid lost updates when multiple threads
target the same index; replace the direct updates (e.g., output[idx] =
output[idx] + forces[i] and applied_output[idx] = applied_output[idx] +
applied_forces[i], computed_output[idx] = computed_output[idx] +
computed_forces[i]) with the Warp atomic add primitive (e.g., wp.atomic.add) so
each thread performs an atomic addition into output, applied_output, and
computed_output respectively.
In `@newton_actuators/_src/controllers/controller_pd.py`:
- Around line 17-24: The kernel _pd_force_kernel currently declares
control_input and constant_force as concrete wp.array kernel params but
compute() may call the kernel with act_input=None or self.constant_force=None
which is unsafe; before launching _pd_force_kernel (and the other kernel
variants referenced around lines 35-41 and 79-111), ensure you materialize
zero-filled wp.array instances when act_input or constant_force is None (e.g.,
create arrays of the correct dtype/shape filled with zeros matching
state_indices/target_indices length) or alternatively provide a separate kernel
overload that omits those parameters; update the caller in compute() to replace
None with those zero arrays so the kernel never receives None for
control_input/constant_force.
In `@newton_actuators/_src/controllers/controller_pid.py`:
- Around line 19-27: The kernel _pid_force_kernel must never be launched with
Python None for array parameters (e.g., control_input, constant_force); update
compute() to ensure all kernel arguments are concrete wp.array instances by
either (a) creating zero/empty wp.array placeholders for any None inputs
(matching expected dtype and length) before calling _pid_force_kernel, or (b)
split into two kernel variants and call the correct one when control_input or
constant_force is None; adjust the call sites in compute() and any places that
pass state_indices/target_indices/force_indices so they always supply valid
wp.array objects rather than None.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: da949fbd-46af-45be-8d65-03b7649f53e1
📒 Files selected for processing (15)
README.mdnewton_actuators/__init__.pynewton_actuators/_src/actuator.pynewton_actuators/_src/clamping/__init__.pynewton_actuators/_src/clamping/base.pynewton_actuators/_src/clamping/clamping_max_force.pynewton_actuators/_src/clamping/clamping_position_based.pynewton_actuators/_src/clamping/clamping_velocity_based.pynewton_actuators/_src/controllers/__init__.pynewton_actuators/_src/controllers/controller_net_lstm.pynewton_actuators/_src/controllers/controller_net_mlp.pynewton_actuators/_src/controllers/controller_pd.pynewton_actuators/_src/controllers/controller_pid.pynewton_actuators/_src/usd_parser.pynewton_actuators/tests/test_actuators.py
✅ Files skipped from review due to trivial changes (1)
- newton_actuators/_src/controllers/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton_actuators/_src/clamping/init.py
| def step( | ||
| self, | ||
| sim_state: Any, | ||
| sim_control: Any, | ||
| current_act_state: StateActuator | None = None, | ||
| next_act_state: StateActuator | None = None, | ||
| dt: float = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Make dt required or validate it upfront.
The public signature makes dt look optional, but compute() and delay.update_state() always receive it. For PID/delay/stateful paths, omitting dt will fail later or propagate None into time-based math. Either remove the default or raise before dispatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/actuator.py` around lines 180 - 187, The step method
currently has an optional dt parameter which is actually required by downstream
callers (compute() and delay.update_state()), so either remove the default to
make dt a required positional/keyword argument on Actuator.step or validate it
at the top of step and raise a clear exception; update the signature on def
step(..., dt: float) -> None to drop the "= None" or add an early check like "if
dt is None: raise ValueError('dt is required')" before any calls to compute() or
delay.update_state() so no None is propagated into time-based math (refer to
step, compute, and delay.update_state in your changes).
| has_states = current_act_state is not None and next_act_state is not None | ||
|
|
||
| positions = getattr(sim_state, self.state_pos_attr) | ||
| velocities = getattr(sim_state, self.state_vel_attr) | ||
|
|
||
| orig_target_pos = getattr(sim_control, self.control_target_pos_attr) | ||
| orig_target_vel = getattr(sim_control, self.control_target_vel_attr) | ||
| orig_act_input = None | ||
| if self.control_input_attr is not None: | ||
| orig_act_input = getattr(sim_control, self.control_input_attr, None) | ||
|
|
||
| target_pos = orig_target_pos | ||
| target_vel = orig_target_vel | ||
| act_input = orig_act_input | ||
| target_indices = self.indices | ||
|
|
||
| # --- 1. Delay: read delayed targets --- | ||
| skip_compute = False | ||
| if self.delay is not None: | ||
| delay_state = current_act_state.delay_state if current_act_state else None | ||
|
|
||
| if self.delay.is_ready(delay_state): | ||
| target_pos, target_vel, act_input, target_indices = ( | ||
| self.delay.get_delayed_targets(act_input, delay_state) | ||
| ) | ||
| else: | ||
| skip_compute = True | ||
|
|
||
| if not skip_compute: | ||
| # --- 2. Controller: compute raw forces --- | ||
| ctrl_state = current_act_state.controller_state if current_act_state else None | ||
| self.controller.compute( | ||
| positions, | ||
| velocities, | ||
| target_pos, | ||
| target_vel, | ||
| act_input, | ||
| self.indices, | ||
| target_indices, | ||
| self._computed_forces, | ||
| self._sequential_indices, | ||
| self.num_actuators, | ||
| ctrl_state, | ||
| dt, |
There was a problem hiding this comment.
Fail fast when a stateful actuator is stepped without both state buffers.
step() still calls into delay.is_ready() / controller.compute() when current_act_state or next_act_state is missing. The new neural controllers dereference their state immediately, so this turns into a late AttributeError inside component code instead of a clear API error at the boundary. Require both buffers whenever self.is_stateful() is true.
Suggested guard
- has_states = current_act_state is not None and next_act_state is not None
+ has_states = current_act_state is not None and next_act_state is not None
+ if self.is_stateful() and not has_states:
+ raise ValueError(
+ "Stateful actuators require both current_act_state and next_act_state"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/actuator.py` around lines 206 - 249, If the actuator is
stateful (self.is_stateful() true), ensure step() checks that both
current_act_state and next_act_state are present and raise a clear API error
immediately (e.g., ValueError/RuntimeError) instead of proceeding; add the guard
at the start of the section that currently sets has_states and before calling
delay.is_ready() or controller.compute(), so delay.is_ready(delay_state) and
self.controller.compute(...) never run when current_act_state or next_act_state
is missing.
| def modify_forces( | ||
| self, | ||
| src_forces: wp.array, | ||
| dst_forces: wp.array, | ||
| positions: wp.array, | ||
| velocities: wp.array, | ||
| input_indices: wp.array, | ||
| num_actuators: int, | ||
| ) -> None: | ||
| """Read forces from src, apply clamping, write to dst. | ||
|
|
||
| When src and dst are the same array, this is an in-place update. | ||
| The Actuator uses different arrays for the first clamping | ||
| (to preserve the raw controller output) and the same array | ||
| for subsequent clampings. | ||
|
|
||
| Args: | ||
| src_forces: Input force buffer to read. Shape (N,). | ||
| dst_forces: Output force buffer to write. Shape (N,). | ||
| positions: Joint positions (global array). | ||
| velocities: Joint velocities (global array). | ||
| input_indices: Indices into positions/velocities. | ||
| num_actuators: Number of actuators N. | ||
| """ | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fail fast in the base modify_forces() contract.
Leaving this as a no-op makes an incomplete custom clamp silently drop force updates. This should raise immediately instead of pretending the override exists.
Suggested change
def modify_forces(
self,
src_forces: wp.array,
dst_forces: wp.array,
positions: wp.array,
velocities: wp.array,
input_indices: wp.array,
num_actuators: int,
) -> None:
"""Read forces from src, apply clamping, write to dst.
@@
velocities: Joint velocities (global array).
input_indices: Indices into positions/velocities.
num_actuators: Number of actuators N.
"""
- pass
+ raise NotImplementedError(f"{type(self).__name__} must implement modify_forces")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/clamping/base.py` around lines 50 - 74, The base class
modify_forces method currently is a silent no-op; change it to fail fast by
raising a clear error (e.g., NotImplementedError) inside modify_forces so
subclasses must implement it: locate the modify_forces method in
newton_actuators._src.clamping.base (the base clamping class) and replace the
empty implementation with a raised exception that includes a descriptive message
referencing modify_forces and the clamping class to aid debugging.
| @classmethod | ||
| def resolve_arguments(cls, args: dict[str, Any]) -> dict[str, Any]: | ||
| return {"max_force": args.get("max_force", math.inf)} | ||
|
|
||
| def __init__(self, max_force: wp.array): | ||
| """Initialize clamp dynamic. | ||
|
|
||
| Args: | ||
| max_force: Per-actuator force limits. Shape (N,). | ||
| """ | ||
| self.max_force = max_force |
There was a problem hiding this comment.
Validate max_force is non-negative.
The kernel assumes each bound is [-max_force[i], +max_force[i]]. If any entry is negative, that interval is reversed and the clamp becomes incorrect. Please reject negative limits in resolve_arguments() and/or __init__().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/clamping/clamping_max_force.py` around lines 29 - 39,
Ensure negative per-actuator limits are rejected by validating max_force in both
resolve_arguments and __init__: in resolve_arguments(cls, args) check
args.get("max_force", math.inf) for any negative entries and raise a ValueError
if so (include the offending value in the message); in __init__(self, max_force:
wp.array) validate the provided wp.array has all elements >= 0 (e.g., use a
vectorized test or iterate to detect any element < 0) and raise ValueError if
any are negative; keep the attribute name max_force and preserve the existing
behavior when values are non-negative.
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from typing import Any | ||
|
|
||
| import numpy as np | ||
| import warp as wp | ||
|
|
||
| from .base import Clamping | ||
|
|
||
|
|
||
| @wp.func | ||
| def _interp_1d( | ||
| x: float, | ||
| xs: wp.array(dtype=float), | ||
| ys: wp.array(dtype=float), | ||
| n: int, | ||
| ) -> float: | ||
| """Linearly interpolate (x -> y) from sorted sample arrays, clamping at boundaries.""" | ||
| if n <= 0: | ||
| return 0.0 | ||
| if x <= xs[0]: | ||
| return ys[0] | ||
| if x >= xs[n - 1]: | ||
| return ys[n - 1] | ||
| for k in range(n - 1): | ||
| if xs[k + 1] >= x: | ||
| dx = xs[k + 1] - xs[k] | ||
| if dx == 0.0: | ||
| return ys[k] | ||
| t = (x - xs[k]) / dx | ||
| return ys[k] + t * (ys[k + 1] - ys[k]) | ||
| return ys[n - 1] | ||
|
|
||
|
|
||
| @wp.kernel | ||
| def _remotized_clamp_kernel( | ||
| current_pos: wp.array(dtype=float), | ||
| state_indices: wp.array(dtype=wp.uint32), | ||
| lookup_angles: wp.array(dtype=float), | ||
| lookup_torques: wp.array(dtype=float), | ||
| lookup_size: int, | ||
| src: wp.array(dtype=float), | ||
| dst: wp.array(dtype=float), | ||
| ): | ||
| """Angle-dependent clamping via interpolated lookup table: read src, write dst.""" | ||
| i = wp.tid() | ||
| state_idx = state_indices[i] | ||
| limit = _interp_1d(current_pos[state_idx], lookup_angles, lookup_torques, lookup_size) | ||
| dst[i] = wp.clamp(src[i], -limit, limit) | ||
|
|
||
|
|
||
| class ClampingPositionBased(Clamping): | ||
| """Angle-dependent torque clamping via lookup table. | ||
|
|
||
| Replaces a fixed ±max_force box clamp with angle-dependent torque | ||
| limits interpolated from a lookup table. Models remotized actuators | ||
| (e.g., linkage-driven joints) where the transmission ratio and thus | ||
| maximum output torque vary with joint angle. | ||
|
|
||
| This is a post-controller dynamic. | ||
| """ | ||
|
|
||
| SHARED_PARAMS = {"lookup_angles", "lookup_torques"} | ||
|
|
||
| @classmethod | ||
| def resolve_arguments(cls, args: dict[str, Any]) -> dict[str, Any]: | ||
| if "lookup_angles" not in args or "lookup_torques" not in args: | ||
| raise ValueError("ClampPositionBased requires 'lookup_angles' and 'lookup_torques' arguments") | ||
| return { | ||
| "lookup_angles": tuple(args["lookup_angles"]), | ||
| "lookup_torques": tuple(args["lookup_torques"]), | ||
| } | ||
|
|
||
| def __init__( | ||
| self, | ||
| lookup_angles: wp.array | tuple[float, ...] | list[float], | ||
| lookup_torques: wp.array | tuple[float, ...] | list[float], | ||
| ): | ||
| """Initialize remotized clamp dynamic. | ||
|
|
||
| Args: | ||
| lookup_angles: Sorted joint angles for the torque lookup table. Shape (K,). | ||
| lookup_torques: Max output torques corresponding to lookup_angles. Shape (K,). | ||
| """ | ||
| if len(lookup_angles) != len(lookup_torques): | ||
| raise ValueError( | ||
| f"lookup_angles length ({len(lookup_angles)}) must match " | ||
| f"lookup_torques length ({len(lookup_torques)})" | ||
| ) | ||
| self.lookup_size = len(lookup_angles) | ||
| self._raw_angles = lookup_angles | ||
| self._raw_torques = lookup_torques | ||
| self.lookup_angles: wp.array | None = None | ||
| self.lookup_torques: wp.array | None = None | ||
|
|
||
| if isinstance(lookup_angles, wp.array): | ||
| self.lookup_angles = lookup_angles | ||
| if isinstance(lookup_torques, wp.array): | ||
| self.lookup_torques = lookup_torques | ||
|
|
||
| def set_device(self, device: wp.Device) -> None: | ||
| if self.lookup_angles is None: | ||
| self.lookup_angles = wp.array( | ||
| np.array(self._raw_angles, dtype=np.float32), device=device | ||
| ) | ||
| if self.lookup_torques is None: | ||
| self.lookup_torques = wp.array( | ||
| np.array(self._raw_torques, dtype=np.float32), device=device | ||
| ) | ||
|
|
||
| def modify_forces( | ||
| self, | ||
| src_forces: wp.array, | ||
| dst_forces: wp.array, | ||
| positions: wp.array, | ||
| velocities: wp.array, | ||
| input_indices: wp.array, | ||
| num_actuators: int, | ||
| ) -> None: | ||
| wp.launch( | ||
| kernel=_remotized_clamp_kernel, | ||
| dim=num_actuators, | ||
| inputs=[ | ||
| positions, | ||
| input_indices, | ||
| self.lookup_angles, | ||
| self.lookup_torques, | ||
| self.lookup_size, | ||
| src_forces, | ||
| ], | ||
| outputs=[dst_forces], | ||
| ) |
There was a problem hiding this comment.
Run ruff format on this module before merge.
CI already reports “file would be reformatted” for this file.
🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Ruff format check failed: file would be reformatted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/clamping/clamping_position_based.py` around lines 1 -
133, This file fails ruff formatting; run the project's auto-formatter (ruff
format) on newton_actuators/_src/clamping/clamping_position_based.py to fix
whitespace, line breaks, and import formatting issues; after reformatting,
ensure the module still compiles and that symbols like _interp_1d,
_remotized_clamp_kernel, and class ClampingPositionBased (resolve_arguments,
set_device, modify_forces) remain unchanged except for formatting edits, then
update the PR.
| torch_target_idx = ( | ||
| self._torch_sequential_indices | ||
| if target_indices is self._warp_sequential_indices | ||
| else self._torch_input_indices | ||
| ) |
There was a problem hiding this comment.
Don't key target indexing off object identity.
This only works when the delayed-target path returns the exact same sequential_indices object. If an equivalent array is passed instead, the code falls back to self._torch_input_indices and reads compact targets with global DOF indices. Please derive the torch index tensor from target_indices itself, or pass an explicit “compact vs global target array” signal instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/controllers/controller_net_lstm.py` around lines 147 -
151, The code currently chooses torch_target_idx by object identity (comparing
target_indices is self._warp_sequential_indices) which fails when an equivalent
array is passed; replace this with logic that derives the torch index tensor
from target_indices itself (or uses an explicit flag). Concretely, add or reuse
a helper like _torch_indices_for(target_indices) that returns the correct tensor
(e.g., by comparing contents with np.array_equal/torch.equal or by constructing
indices from target_indices values) and use torch_target_idx =
self._torch_indices_for(target_indices) instead of the identity check between
_torch_sequential_indices and _warp_sequential_indices; alternatively accept an
explicit parameter (e.g., targets_are_sequential) and branch on that rather than
object identity.
| torch_target_idx = ( | ||
| self._torch_sequential_indices | ||
| if target_indices is self._warp_sequential_indices | ||
| else self._torch_input_indices | ||
| ) |
There was a problem hiding this comment.
Avoid identity-based target-index dispatch here too.
This branch assumes the compact-target case can be identified by target_indices is self._warp_sequential_indices. If that object identity ever changes, you'll index the compact target buffer with global actuator indices and get wrong inputs. Convert target_indices itself to a torch tensor, or make the caller pass an explicit indexing mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/controllers/controller_net_mlp.py` around lines 135 -
139, Replace the identity check that picks torch_target_idx (currently using
"target_indices is self._warp_sequential_indices") with a stable content-based
approach: convert incoming target_indices to a torch tensor (e.g.,
torch.as_tensor(target_indices)) or accept an explicit indexing-mode flag from
the caller, then select between self._torch_sequential_indices and
self._torch_input_indices based on the content/flag. Update the logic around
torch_target_idx, target_indices, self._warp_sequential_indices,
self._torch_sequential_indices and self._torch_input_indices so indexing no
longer relies on object identity.
| kwargs: dict[str, Any] = {} | ||
| for usd_name, kwarg_name in entry.param_map.items(): | ||
| value = get_attribute(prim, f"newton:actuator:{usd_name}") | ||
| if value is not None: | ||
| kwargs[kwarg_name] = value | ||
|
|
||
| transmission = get_attribute(prim, "newton:actuator:transmission") | ||
| if entry.validate is not None: | ||
| entry.validate(kwargs) | ||
|
|
||
| if entry.is_controller: | ||
| if controller_class is not None: | ||
| raise ValueError( | ||
| f"Actuator prim has multiple controllers: " | ||
| f"{controller_class.__name__} and {entry.component_class.__name__}" | ||
| ) | ||
| controller_class = entry.component_class | ||
| controller_kwargs = kwargs | ||
| else: | ||
| component_specs.append((entry.component_class, kwargs)) |
There was a problem hiding this comment.
Reject incomplete applied schemas during parsing.
This loop accepts any applied schema even when none of its required attributes are authored, so you can end up returning controller_class with {} or component specs like (Delay, {}). That defers the failure until later construction and loses the prim/schema context that caused it. Please restore per-schema required-field validation here, especially for schemas such as ControllerNetMLPAPI / ControllerNetLSTMAPI where networkPath is mandatory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton_actuators/_src/usd_parser.py` around lines 136 - 154, The parser
currently accepts an applied schema even when none of its required attributes
are authored; to fix this, after collecting kwargs from entry.param_map but
before treating the schema as present, invoke entry.validate(kwargs)
unconditionally (or check explicitly if no params were authored and then call
validate) and propagate/raise any validation error with context (include prim
path/name and entry.component_class.__name__) so the failure is tied to the
prim/schema; only when validate succeeds should you set
controller_class/controller_kwargs or append to component_specs (using the
existing symbols entry.param_map, get_attribute, entry.validate,
entry.is_controller, controller_class, controller_kwargs, component_specs, and
entry.component_class).
| ``` | ||
| Actuator | ||
| ├── Controller (control law that computes raw forces) | ||
| ├── Delay (optional: delays control targets by N timesteps) | ||
| └── Clamping[] (post-controller force bounds) | ||
| ├── ClampingMaxForce (±max_force box clamp) | ||
| ├── ClampingVelocityBased (velocity-dependent saturation) | ||
| └── ClampingPositionBased (angle-dependent lookup) | ||
| ``` |
There was a problem hiding this comment.
Add a language to this fenced block.
markdownlint is already flagging this block. Mark it as text (or another suitable language) so CI passes cleanly.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 24 - 32, The fenced code block showing the Actuator
tree (lines containing "Actuator", "Controller", "Delay", "Clamping",
"ClampingMaxForce", "ClampingVelocityBased", "ClampingPositionBased") should
include a language tag to satisfy markdownlint; update the triple-backtick fence
to use a language like "text" (or another suitable language) immediately after
the opening ``` so the block is fenced as ```text.
| state = actuator.state() | ||
| for step in range(num_steps): | ||
| lstm_actuator.step(sim_state, sim_control, state, state, dt=0.01) | ||
| actuator.step(sim_state, sim_control, state, state, dt=0.01) |
There was a problem hiding this comment.
Use double-buffered state in the LSTM example.
The workflow above documents current/next buffers for stateful actuators, but this example passes the same state as both inputs. Showing the double-buffered pattern here will keep the docs aligned with the rest of the API and avoid readers cargo-culting in-place state updates.
Suggested doc update
-state = actuator.state()
+state_a = actuator.state()
+state_b = actuator.state()
+current, nxt = state_a, state_b
for step in range(num_steps):
- actuator.step(sim_state, sim_control, state, state, dt=0.01)
+ actuator.step(sim_state, sim_control, current, nxt, dt=0.01)
+ current, nxt = nxt, current📝 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.
| state = actuator.state() | |
| for step in range(num_steps): | |
| lstm_actuator.step(sim_state, sim_control, state, state, dt=0.01) | |
| actuator.step(sim_state, sim_control, state, state, dt=0.01) | |
| state_a = actuator.state() | |
| state_b = actuator.state() | |
| current, nxt = state_a, state_b | |
| for step in range(num_steps): | |
| actuator.step(sim_state, sim_control, current, nxt, dt=0.01) | |
| current, nxt = nxt, current |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 214 - 216, The example uses the same buffer for both
current and next state when calling actuator.step, which breaks the
double-buffered pattern; update the docs to call actuator.state() to create two
buffers (e.g., state and next_state), pass state as the current state and
next_state as the next state to actuator.step(sim_state, sim_control, state,
next_state, dt=0.01), then after each step swap or reassign so the next_state
becomes the current for the following iteration; reference the actuator.state()
factory and actuator.step(sim_state, sim_control, current_state, next_state, dt)
when describing the pattern.
There was a problem hiding this comment.
♻️ Duplicate comments (8)
newton_actuators/_src/actuator.py (3)
18-42:⚠️ Potential issue | 🟠 MajorUse atomic accumulation in both scatter kernels.
Warp provides
wp.atomic_add()for per-element atomic updates, and the docs note thatarr[i] += valuemaps to the same primitive. Both kernels currently do non-atomic read-modify-write accumulation, so duplicate actuator indices can drop contributions nondeterministically. (nvidia.github.io)Suggested fix
def _scatter_add_kernel( @@ """Scatter-add forces into output at specified indices.""" i = wp.tid() idx = indices[i] - output[idx] = output[idx] + forces[i] + wp.atomic_add(output, idx, forces[i]) @@ def _scatter_add_dual_kernel( @@ """Scatter-add both applied and computed forces in one pass.""" i = wp.tid() idx = indices[i] - applied_output[idx] = applied_output[idx] + applied_forces[i] - computed_output[idx] = computed_output[idx] + computed_forces[i] + wp.atomic_add(applied_output, idx, applied_forces[i]) + wp.atomic_add(computed_output, idx, computed_forces[i])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuator.py` around lines 18 - 42, The scatter kernels perform non-atomic read-modify-write updates which can lose contributions for duplicate indices; in _scatter_add_kernel replace the non-atomic accumulation of output[idx] = output[idx] + forces[i] with a call to wp.atomic_add(output, idx, forces[i]) (using the same signature Warp expects), and in _scatter_add_dual_kernel replace both applied_output[idx] = applied_output[idx] + applied_forces[i] and computed_output[idx] = computed_output[idx] + computed_forces[i] with wp.atomic_add(applied_output, idx, applied_forces[i]) and wp.atomic_add(computed_output, idx, computed_forces[i]) respectively so each update is performed atomically.
180-187:⚠️ Potential issue | 🟠 MajorMake
dtrequired at the API boundary.The signature still advertises
dtas optional, butstep()forwards it into time-based controller and delay code. Omitting it turns into a late failure orNoneleaking into math instead of a clear API error.Suggested fix
def step( self, sim_state: Any, sim_control: Any, current_act_state: StateActuator | None = None, next_act_state: StateActuator | None = None, - dt: float = None, + dt: float, ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuator.py` around lines 180 - 187, The step(...) API currently declares dt: float = None but forwards it into time-based controllers and delay logic, allowing None to leak into math; make dt required at the API boundary by changing the signature of step in the Actuator class/method (step) to accept dt: float (non-optional), remove the default None, and add a defensive check that raises a clear TypeError if dt is None when called; update any callers of step to pass a float dt accordingly and adjust type hints for StateActuator-related uses if needed so callers and downstream methods (controllers/delay code) never receive None.
206-207:⚠️ Potential issue | 🟠 MajorFail fast when a stateful actuator is stepped without both buffers.
If the controller or delay is stateful, this path still enters component code with missing state or silently skips state advancement. That turns a boundary misuse into late
AttributeErrors or permanent no-op delay filling instead of a clear error.Suggested fix
has_states = current_act_state is not None and next_act_state is not None + if self.is_stateful() and not has_states: + raise ValueError( + "Stateful actuators require both current_act_state and next_act_state" + )Also applies to: 225-249, 287-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuator.py` around lines 206 - 207, The code computes has_states = current_act_state is not None and next_act_state is not None but doesn't fail fast when a stateful component is stepped without both buffers; update the stepping logic in the Actuator (the method that defines has_states/current_act_state/next_act_state and the surrounding branches at the same pattern around lines matching 225-249 and 287-301) to validate early: if the controller or delay is stateful (check the same condition you use to decide to use states) and either current_act_state or next_act_state is None, raise a clear ValueError (or custom exception) indicating the missing state buffer and which one is absent, rather than proceeding or silently skipping state advancement. Ensure the error is raised before any component code runs so misuse is detected immediately.README.md (2)
214-216:⚠️ Potential issue | 🟡 MinorUse two state buffers in the LSTM example.
Passing the same object as both current and next state at Line 216 contradicts the double-buffered workflow documented above and encourages in-place updates for a stateful actuator.
Suggested fix
-state = actuator.state() +state_a = actuator.state() +state_b = actuator.state() +current, nxt = state_a, state_b for step in range(num_steps): - actuator.step(sim_state, sim_control, state, state, dt=0.01) + actuator.step(sim_state, sim_control, current, nxt, dt=0.01) + current, nxt = nxt, current🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 214 - 216, The snippet passes the same buffer for current and next state to actuator.step (creating in-place updates), which breaks the double-buffered pattern; change to allocate two distinct buffers via actuator.state() (e.g., state and next_state), call actuator.step(sim_state, sim_control, state, next_state, dt=0.01), then swap or assign next_state to state each loop iteration to preserve the double-buffered workflow used by the LSTM actuator.
24-32:⚠️ Potential issue | 🟡 MinorAdd a language to this fenced block.
markdownlintis already flagging the fence at Line 24, so CI will keep complaining until this tree example is fenced astext(or similar).Suggested fix
-``` +```text Actuator ├── Controller (control law that computes raw forces) ├── Delay (optional: delays control targets by N timesteps) └── Clamping[] (post-controller force bounds) ├── ClampingMaxForce (±max_force box clamp) ├── ClampingDCMotor (velocity-dependent saturation) └── ClampingPositionBased (angle-dependent lookup)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 24 - 32, The fenced code block showing the Actuator
tree is missing a language tag; update the fence that starts before "Actuator"
and ends after "ClampingPositionBased" to include a language like text (e.g."Actuator" header and change the opening ``` to ```text and keep the contents unchanged.newton_actuators/_src/controllers/controller_pid.py (1)
19-27:⚠️ Potential issue | 🔴 CriticalDon't pass
Noneinto concrete Warp array parameters.Warp documents that kernel arguments are statically typed, and that
typing.Anyis the generic escape hatch when a kernel needs multiple concrete instantiations. Given that_pid_force_kerneldeclares concretewp.array(...)slots, forwarding PythonNoneforact_inputorself.constant_forcefromcompute()is not a supported launch pattern, so the defaultControllerPID(..., constant_force=None)path can fail before those kernel-sideifchecks help. Materialize zero-filled arrays before launch and make the kernel read them unconditionally. (nvidia.github.io)Suggested fix
`@wp.kernel` def _pid_force_kernel( @@ - const_f = float(0.0) - if constant_force: - const_f = constant_force[i] - - act = float(0.0) - if control_input: - act = control_input[target_idx] + const_f = constant_force[i] + act = control_input[target_idx] @@ class ControllerPID(Controller): @@ self.integral_max = integral_max self.constant_force = constant_force self._next_integral: wp.array | None = None + self._zero_act_input: wp.array | None = None def set_indices(self, input_indices: wp.array, sequential_indices: wp.array) -> None: num = len(input_indices) device = input_indices.device self._next_integral = wp.zeros(num, dtype=wp.float32, device=device) + self._zero_act_input = wp.zeros(num, dtype=wp.float32, device=device) + if self.constant_force is None: + self.constant_force = wp.zeros(num, dtype=wp.float32, device=device) @@ def compute( @@ ) -> None: + act_input = act_input if act_input is not None else self._zero_act_input wp.launch( kernel=_pid_force_kernel, dim=num_actuators,Also applies to: 45-51, 91-92, 137-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/controllers/controller_pid.py` around lines 19 - 27, The kernel _pid_force_kernel and related warp kernels declare concrete wp.array parameters but compute() may forward Python None for act_input or self.constant_force; change ControllerPID.__init__/compute() to always materialize zero-filled wp.array buffers (dtype matching kp/ki/kd and same length as relevant indices) instead of passing None, and update compute() to pass those arrays unconditionally to _pid_force_kernel (and any kernels referenced at lines ~45-51, 91-92, 137-158) so the kernel reads arrays without relying on Python-side None checks; ensure the arrays are allocated with wp.zeros or equivalent with correct dtype (float or wp.uint32 where appropriate) and lengths matching state_indices/target_indices/force_indices.newton_actuators/_src/clamping/clamping_dc_motor.py (1)
31-35:⚠️ Potential issue | 🟠 MajorGuard the torque-speed math against invalid or infinite limits.
velocity_limitis only checked for presence here, but the kernel divides by it, so zero or negative values still produce bad saturation whenClampingDCMotor(...)is constructed directly. Themath.infdefault forsaturation_effortis also unsafe in this formula, because the cutoff case becomesinf * 0and can turn the clamp bounds intoNaN. Please validate positivevelocity_limitin the component itself and special-case the unbounded path instead of feeding raw infinities into the kernel.Also applies to: 52-59, 61-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/clamping/clamping_dc_motor.py` around lines 31 - 35, The kernel that computes max_torque/min_torque uses division by velocity_limit and multiplies by saturation_effort, which can be zero/negative or math.inf and produce NaN; update the ClampingDCMotor constructor (or component validation) to assert/raise if any velocity_limit values are <= 0 and treat saturation_effort==math.inf as the unbounded path instead of passing infinities into the kernel, and then change the kernel logic (the computation around max_torque/min_torque in clamping_dc_motor.py) to special-case the unbounded saturation: if saturation is unbounded skip the sat*(...)/vel_lim math and use ±max_force directly (or otherwise compute safe finite bounds), ensuring no division by zero or inf*0 occurs; apply the same guard/fix to the other similar blocks referenced (around the other torque-speed computations in the file: lines ~52-59 and ~61-76).newton_actuators/_src/usd_parser.py (1)
24-68:⚠️ Potential issue | 🟠 MajorReject incomplete applied schemas at parse time.
This path only enforces the validators that exist in
SCHEMA_REGISTRY, soDelayAPI,ControllerNetMLPAPI, andControllerNetLSTMAPIcan still parse as empty kwargs and fail later during construction. Add required-field validators for schemas with mandatory authored attributes and wrap the resultingValueErrorwith the prim path/schema name so bad USD is rejected with context.Suggested fix
+def _require_keys(schema_name: str, *keys: str): + def validate(kwargs: dict[str, Any]) -> None: + missing = [key for key in keys if key not in kwargs] + if missing: + raise ValueError(f"{schema_name} requires {', '.join(missing)}") + return validate + + def _validate_clamp_velocity_based(kwargs: dict[str, Any]) -> None: vel_lim = kwargs.get("velocity_limit") if vel_lim is not None and vel_lim <= 0.0: raise ValueError( f"ClampingDCMotorAPI requires velocity_limit > 0 (division by velocity_limit " @@ "DelayAPI": SchemaEntry( component_class=Delay, param_map={"delay": "delay"}, + validate=_require_keys("DelayAPI", "delay"), ), @@ "ControllerNetMLPAPI": SchemaEntry( component_class=ControllerNetMLP, param_map={"networkPath": "network_path"}, is_controller=True, + validate=_require_keys("ControllerNetMLPAPI", "network_path"), ), "ControllerNetLSTMAPI": SchemaEntry( component_class=ControllerNetLSTM, param_map={"networkPath": "network_path"}, is_controller=True, + validate=_require_keys("ControllerNetLSTMAPI", "network_path"), ), } @@ - if entry.validate is not None: - entry.validate(kwargs) + if entry.validate is not None: + try: + entry.validate(kwargs) + except ValueError as exc: + raise ValueError( + f"{prim.GetPath()}: invalid {schema_name} config: {exc}" + ) from excAlso applies to: 142-154
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@newton_actuators/_src/actuator.py`:
- Around line 18-42: The scatter kernels perform non-atomic read-modify-write
updates which can lose contributions for duplicate indices; in
_scatter_add_kernel replace the non-atomic accumulation of output[idx] =
output[idx] + forces[i] with a call to wp.atomic_add(output, idx, forces[i])
(using the same signature Warp expects), and in _scatter_add_dual_kernel replace
both applied_output[idx] = applied_output[idx] + applied_forces[i] and
computed_output[idx] = computed_output[idx] + computed_forces[i] with
wp.atomic_add(applied_output, idx, applied_forces[i]) and
wp.atomic_add(computed_output, idx, computed_forces[i]) respectively so each
update is performed atomically.
- Around line 180-187: The step(...) API currently declares dt: float = None but
forwards it into time-based controllers and delay logic, allowing None to leak
into math; make dt required at the API boundary by changing the signature of
step in the Actuator class/method (step) to accept dt: float (non-optional),
remove the default None, and add a defensive check that raises a clear TypeError
if dt is None when called; update any callers of step to pass a float dt
accordingly and adjust type hints for StateActuator-related uses if needed so
callers and downstream methods (controllers/delay code) never receive None.
- Around line 206-207: The code computes has_states = current_act_state is not
None and next_act_state is not None but doesn't fail fast when a stateful
component is stepped without both buffers; update the stepping logic in the
Actuator (the method that defines has_states/current_act_state/next_act_state
and the surrounding branches at the same pattern around lines matching 225-249
and 287-301) to validate early: if the controller or delay is stateful (check
the same condition you use to decide to use states) and either current_act_state
or next_act_state is None, raise a clear ValueError (or custom exception)
indicating the missing state buffer and which one is absent, rather than
proceeding or silently skipping state advancement. Ensure the error is raised
before any component code runs so misuse is detected immediately.
In `@newton_actuators/_src/clamping/clamping_dc_motor.py`:
- Around line 31-35: The kernel that computes max_torque/min_torque uses
division by velocity_limit and multiplies by saturation_effort, which can be
zero/negative or math.inf and produce NaN; update the ClampingDCMotor
constructor (or component validation) to assert/raise if any velocity_limit
values are <= 0 and treat saturation_effort==math.inf as the unbounded path
instead of passing infinities into the kernel, and then change the kernel logic
(the computation around max_torque/min_torque in clamping_dc_motor.py) to
special-case the unbounded saturation: if saturation is unbounded skip the
sat*(...)/vel_lim math and use ±max_force directly (or otherwise compute safe
finite bounds), ensuring no division by zero or inf*0 occurs; apply the same
guard/fix to the other similar blocks referenced (around the other torque-speed
computations in the file: lines ~52-59 and ~61-76).
In `@newton_actuators/_src/controllers/controller_pid.py`:
- Around line 19-27: The kernel _pid_force_kernel and related warp kernels
declare concrete wp.array parameters but compute() may forward Python None for
act_input or self.constant_force; change ControllerPID.__init__/compute() to
always materialize zero-filled wp.array buffers (dtype matching kp/ki/kd and
same length as relevant indices) instead of passing None, and update compute()
to pass those arrays unconditionally to _pid_force_kernel (and any kernels
referenced at lines ~45-51, 91-92, 137-158) so the kernel reads arrays without
relying on Python-side None checks; ensure the arrays are allocated with
wp.zeros or equivalent with correct dtype (float or wp.uint32 where appropriate)
and lengths matching state_indices/target_indices/force_indices.
In `@README.md`:
- Around line 214-216: The snippet passes the same buffer for current and next
state to actuator.step (creating in-place updates), which breaks the
double-buffered pattern; change to allocate two distinct buffers via
actuator.state() (e.g., state and next_state), call actuator.step(sim_state,
sim_control, state, next_state, dt=0.01), then swap or assign next_state to
state each loop iteration to preserve the double-buffered workflow used by the
LSTM actuator.
- Around line 24-32: The fenced code block showing the Actuator tree is missing
a language tag; update the fence that starts before "Actuator" and ends after
"ClampingPositionBased" to include a language like text (e.g. ```text) so
markdownlint stops flagging it; locate the block containing the "Actuator"
header and change the opening ``` to ```text and keep the contents unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0b47c31-cd6d-4189-8231-e44b888eab28
📒 Files selected for processing (8)
README.mdnewton_actuators/__init__.pynewton_actuators/_src/actuator.pynewton_actuators/_src/clamping/__init__.pynewton_actuators/_src/clamping/clamping_dc_motor.pynewton_actuators/_src/controllers/controller_pid.pynewton_actuators/_src/usd_parser.pynewton_actuators/tests/test_actuators.py
🚧 Files skipped from review as they are similar to previous changes (2)
- newton_actuators/_src/clamping/init.py
- newton_actuators/tests/test_actuators.py
|
|
||
|
|
||
| @dataclass | ||
| class StateActuator: |
There was a problem hiding this comment.
Can we make this class also nested under Actuator, i.e. Actuator.State? Just like you have it for Delay.State.
eric-heiden
left a comment
eric-heiden
left a comment
There was a problem hiding this comment.
Overall looks good to me, the modular design is great!
When merging into Newton the type hints should be cleaned up, i.e. use wp.array[dtype] notation rather than wp.array(dtype=...). There are a bunch of linter errors. It would be nice to avoid Any type hints but try to be more precise (e.g. Controller.State could be an empty dataclass with the def reset(self) -> None: ... definition by default and used as base class for specific controller state classes. Then the controller_state in ActuatorState can use that Controller.State as type hint. We can iterate on this further once you merge into Newton.
There are also a couple of CodeRabbit comments that would be good to address.
4 participants
Summary by CodeRabbit
Release Notes
Breaking Changes
Actuatorarchitecture. Update imports and code to useActuator(controller=..., delay=..., clamping=[...])instead ofActuatorPD,ActuatorPID, etc.New Features
Controller,Delay, andClampingmodules enable flexible force computation pipelines.Documentation