Move modular design of newton-actuators to Newton repo#2449
Merged
adenzler-nvidia merged 43 commits intonewton-physics:mainfrom Apr 23, 2026
Merged
Move modular design of newton-actuators to Newton repo#2449adenzler-nvidia merged 43 commits intonewton-physics:mainfrom
adenzler-nvidia merged 43 commits intonewton-physics:mainfrom
Conversation
Signed-off-by: jprozorova <jprozorova@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the external actuator package with an in-repo actuator subsystem: adds Actuator core, Controller implementations (PD, PID, MLP, LSTM), Clamping variants, Delay buffering, USD actuator parsing, builder integration, tests, docs, and removes the external Changes
Sequence Diagram(s)sequenceDiagram
participant User as Simulation User
participant Builder as ModelBuilder
participant Parser as USD Parser
participant Actuator as Actuator.step()
participant Delay as Delay
participant Controller as Controller
participant Clamping as Clamping
participant Warp as Warp GPU
User->>Builder: add_actuator(controller_class, index, clamping, delay)
Builder->>Builder: group shared vs per-DOF args, accumulate indices
User->>Parser: parse_actuator_prim(usd_prim)
Parser-->>User: ActuatorParsed(controller_class, controller_kwargs, component_specs)
User->>Actuator: step(sim_state, sim_control, current_state, dt)
alt Delay present and not ready
Actuator-->>Delay: update_state only
Actuator-->>User: skip compute (no forces)
else
Actuator->>Delay: get_delayed_targets(current_state)
Delay-->>Actuator: delayed_targets
Actuator->>Controller: compute(positions, velocities, targets, inputs, indices, forces_buf)
Controller->>Warp: launch kernel or run PyTorch inference
Warp-->>Controller: raw forces
Actuator->>Clamping: modify_forces(raw_forces, applied_forces, positions, velocities, indices)
Clamping->>Warp: launch clamp kernel
Warp-->>Clamping: clamped forces
Actuator->>Warp: scatter-add applied_forces into sim_control
Warp-->>Actuator: sim_control updated
Actuator->>Controller: update_state(current, next) if stateful
Actuator->>Delay: update_state(targets, inputs, input_indices, current, next)
Actuator-->>User: return (states updated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
newton/_src/actuators/controllers/controller_net_mlp.py (1)
37-39: Consider adding a None check inState.reset().If
reset()is called beforestate()initializes the tensors, calling.zero_()onNonewill raiseAttributeError. While the current flow should prevent this, a defensive check would improve robustness.🛡️ Proposed defensive check
def reset(self) -> None: + if self.pos_error_history is not None: + self.pos_error_history.zero_() + if self.vel_history is not None: + self.vel_history.zero_() - self.pos_error_history.zero_() - self.vel_history.zero_()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_mlp.py` around lines 37 - 39, In State.reset(), guard against attributes being None before calling in-place zeroing: check that self.pos_error_history and self.vel_history are not None (or use truthy check) before calling .zero_() so reset() no-ops safely if tensors haven't been initialized; update the reset method (referenced as reset and attributes pos_error_history, vel_history) to perform these None checks.newton/_src/actuators/usd_parser.py (1)
143-146: Redundant None check:schemasis already filtered to registry keys.Line 122 filters
schemasto only include names present inSCHEMA_REGISTRY, soSCHEMA_REGISTRY.get(schema_name)will never returnNone. The check is dead code.♻️ Proposed simplification
for schema_name in schemas: - entry = SCHEMA_REGISTRY.get(schema_name) - if entry is None: - continue + entry = SCHEMA_REGISTRY[schema_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 143 - 146, The loop contains a redundant None check: since `schemas` was previously filtered to keys present in `SCHEMA_REGISTRY`, `SCHEMA_REGISTRY.get(schema_name)` cannot return None; remove the `if entry is None: continue` branch and either use the direct lookup `entry = SCHEMA_REGISTRY[schema_name]` or keep `get` but proceed without the dead check in the loop that iterates over `schemas` (referencing `schemas`, `SCHEMA_REGISTRY.get`, and the `entry` variable).newton/_src/actuators/clamping/base.py (1)
50-73: Add SI units tomodify_forcesdocstring and consider annotating array dtypes.Per coding guidelines, physical quantities should include SI units. Also,
wp.arrayannotations should use bracket syntax to specify dtype.📝 Proposed improvements
def modify_forces( self, - src_forces: wp.array, - dst_forces: wp.array, - positions: wp.array, - velocities: wp.array, - input_indices: wp.array, + src_forces: wp.array[float], + dst_forces: wp.array[float], + positions: wp.array[float], + velocities: wp.array[float], + input_indices: wp.array[Any], 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). + src_forces: Input force buffer [N or N·m] to read. Shape (N,). + dst_forces: Output force buffer [N or N·m] to write. Shape (N,). + positions: Joint positions [m or rad] (global array). + velocities: Joint velocities [m/s or rad/s] (global array). input_indices: Indices into positions/velocities. num_actuators: Number of actuators N. """As per coding guidelines: "Annotate Warp arrays with bracket syntax" and "Include SI units for physical quantities in public API docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/clamping/base.py` around lines 50 - 73, The modify_forces method docstring is missing SI units for physical quantities and the wp.array type annotations lack dtype brackets; update the docstring for modify_forces to state SI units for forces (e.g., newtons, N), positions (meters or radians as appropriate), and velocities (m/s or rad/s), and change the type hints for src_forces, dst_forces, positions, velocities, and input_indices to use bracketed Warp array dtypes (e.g., wp.array[float32] or wp.array[int32] as appropriate) while leaving num_actuators as int; ensure the docstring units match the semantics used elsewhere in the Actuator implementation.newton/_src/actuators/clamping/clamping_position_based.py (1)
82-85: Add SI units to docstring for physical quantities.Per coding guidelines, public API docstrings should include SI units.
lookup_anglesshould specify[rad]andlookup_torquesshould specify[N·m].📝 Proposed docstring update
Args: - lookup_angles: Sorted joint angles for the torque lookup table. Shape (K,). - lookup_torques: Max output torques corresponding to lookup_angles. Shape (K,). + lookup_angles: Sorted joint angles [rad] for the torque lookup table. Shape (K,). + lookup_torques: Max output torques [N·m] corresponding to lookup_angles. Shape (K,).As per coding guidelines: "Include SI units for physical quantities in public API docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/clamping/clamping_position_based.py` around lines 82 - 85, Update the public docstring for the clamping position-based API to include SI units for the physical quantities: annotate lookup_angles as in radians (e.g., lookup_angles [rad]) and lookup_torques as in newton-metres (e.g., lookup_torques [N·m]) in the Args section of the constructor/function (search for lookup_angles and lookup_torques in ClampingPositionBased / clamping_position_based.py) so the docstring clearly states the units.newton/_src/actuators/controllers/controller_net_lstm.py (2)
175-183: Guard againstcurrent_statebeingNoneinupdate_state.The method checks
next_state is Nonebut doesn't checkcurrent_state. Ifcurrent_stateisNone, the method would still try to accessself._hiddenandself._cellwhich are set incompute(). This works ifcompute()was called first, but the method signature suggests both states could be optional.🛡️ Suggested improvement for clarity
def update_state( self, current_state: ControllerNetLSTM.State, next_state: ControllerNetLSTM.State, ) -> None: - if next_state is None: + if next_state is None or self._hidden is None: return next_state.hidden = self._hidden next_state.cell = self._cellThis guards against the edge case where
update_stateis called without a priorcompute()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 175 - 183, The update_state method currently only guards next_state but not current_state; modify ControllerNetLSTM.update_state to also check if current_state is None (or if self._hidden/self._cell are uninitialized) and return early if so, to avoid using self._hidden and self._cell when compute() wasn't called; reference the method name update_state, parameters current_state and next_state, and the instance attributes self._hidden and self._cell when adding the guard and returning early.
132-173: Object identity check for index selection may be fragile.Line 155 uses
target_indices is self._warp_sequential_indicesto determine which torch indices to use. Object identity checks can be unreliable if the array is copied or recreated elsewhere. Consider using a more robust comparison or documenting this contract.💡 Alternative approach
If the identity check is intentional for performance (avoiding array comparison), document the contract clearly:
+ # Identity check: when delay is active, target_indices points to + # sequential_indices (local indexing). When not delayed, it points + # to input_indices (global indexing). torch_target_idx = ( self._torch_sequential_indices if target_indices is self._warp_sequential_indices else self._torch_input_indices )Or use a flag passed from the caller to indicate which indexing mode is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 132 - 173, In compute, avoid fragile object-identity selection using `target_indices is self._warp_sequential_indices`; instead determine the correct torch index set by a robust check or explicit flag: either compare index contents (e.g., element-wise/equality check between `target_indices` and `self._warp_sequential_indices` or convert to torch and use `torch.equal`) or add an explicit boolean parameter (e.g., `use_sequential_indices`) passed from the caller and branch on that; update the selection logic for `torch_target_idx` (currently choosing between `self._torch_sequential_indices` and `self._torch_input_indices`) and adjust call sites to provide the flag if you take that approach.newton/_src/actuators/actuator.py (1)
160-168: Consider makingSHARED_PARAMSa class variable instead of a property.
SHARED_PARAMSis named like a constant (uppercase) but implemented as a property that recomputes the set on every access. This is inconsistent with theClassVar[set[str]]pattern used in other classes likeDelayandController.♻️ Suggested refactor
If the composed set must be dynamic, consider caching it or documenting that this is intentional. Alternatively, compute it once during
__init__:def __init__( self, indices: wp.array, controller: Controller, delay: Delay | None = None, clamping: list[Clamping] | None = None, ... ): ... + # Compute shared params once + self._shared_params: set[str] = set() + self._shared_params |= self.controller.SHARED_PARAMS + if self.delay is not None: + self._shared_params |= self.delay.SHARED_PARAMS + for c in self.clamping: + self._shared_params |= c.SHARED_PARAMS - `@property` - def SHARED_PARAMS(self) -> set[str]: - params: set[str] = set() - params |= self.controller.SHARED_PARAMS - if self.delay is not None: - params |= self.delay.SHARED_PARAMS - for c in self.clamping: - params |= c.SHARED_PARAMS - return params + `@property` + def SHARED_PARAMS(self) -> set[str]: + return self._shared_params🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/actuator.py` around lines 160 - 168, The SHARED_PARAMS attribute is implemented as a recomputed `@property` but named like a constant; change it to a class variable or compute/cache it once in the instance to match the ClassVar pattern used by Delay/Controller. Modify SHARED_PARAMS from a property to either a ClassVar[set[str]] (if static) or compute the composed set in __init__ (using self.controller.SHARED_PARAMS, self.delay.SHARED_PARAMS when delay is not None, and each c.SHARED_PARAMS from self.clamping) and store it on the instance (e.g., self.SHARED_PARAMS) so subsequent accesses do not recompute; if dynamic updates are required, add explicit invalidation or lazy caching instead. Ensure references to SHARED_PARAMS elsewhere continue to work with the new class/instance attribute semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/clamping/clamping_dc_motor.py`:
- Around line 51-59: In resolve_arguments of ClampingDCMotor validate that
"velocity_limit" is present and strictly > 0 (raise ValueError if <= 0) and also
prevent the default saturation_effort from being math.inf (either require a
finite saturation_effort or set a safe finite default) to avoid an inf * 0 path;
update the returned dict to use the validated velocity_limit and a finite
saturation_effort and reference the "saturation_effort" and "velocity_limit"
keys in your checks.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Line 69: The error message uses the wrong class name "ClampPositionBased";
update the string so it matches the actual class name `ClampingPositionBased`
(in the constructor/initializer where the ValueError is raised) and search for
any other occurrences of "ClampPositionBased" in the same file to correct them
as well.
- Around line 96-105: The code may leave pre-allocated wp.array objects
(self.lookup_angles, self.lookup_torques) on the wrong device; update set_device
to ensure device consistency by checking the device of any existing wp.array and
migrating/copying it to the target device if it differs (or raising a clear
error); specifically inspect self.lookup_angles and self.lookup_torques inside
set_device (and use wp.array(..., device=...) or a device-aware copy/migrate API
when needed) so kernels launched later won’t see device mismatches.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 164-166: ControllerNetMLP currently ignores the documented
force_indices contract by doing wp.copy(forces, torques_wp) (see the
torques.reshape, torques_wp creation and wp.copy call); change this to write
into forces via force_indices like ControllerPD/ControllerPID: create a small
warp kernel (or reuse the existing indexed-write kernel) that takes forces,
force_indices and torques_wp and does forces[force_indices[i]] = torques_wp[i],
and call that instead of wp.copy; alternatively, if you intentionally rely on
sequential indexing, add a clear comment on ControllerNetMLP and its constructor
documenting that force_indices must be sequential and remove/validate the
force_indices parameter accordingly.
In `@newton/_src/actuators/delay.py`:
- Around line 67-72: reset() currently assumes self.buffer_pos, self.buffer_vel,
and self.buffer_act are non-None and calls .zero_() and .shape on them, which
will raise AttributeError when those attributes are None; update reset() to
first check for None (e.g. if any(b is None for b in (self.buffer_pos,
self.buffer_vel, self.buffer_act)): set self.is_filled = False, set
self.write_idx = -1 or another safe sentinel, and return) otherwise call
.zero_() and compute write_idx as before; alternatively, if the class invariant
guarantees buffers are never None after initialization, remove the | None from
the type hints for buffer_pos/buffer_vel/buffer_act to make the contract
explicit.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 168-171: The error raised when controller_class is None uses
prim.GetAppliedSchemas(), which per earlier notes won't include custom schemas
and is misleading; update the ValueError in usd_parser (the block around
controller_class check) to instead report the schemas discovered from metadata
(the same metadata lookup used earlier around lines 107-109), e.g., include the
list returned by that metadata-based schema extraction or a clear statement that
only metadata-derived schemas were checked, and remove or replace the
GetAppliedSchemas() call so the message accurately reflects what was inspected.
In `@newton/_src/sim/builder.py`:
- Line 1774: The code currently appends the caller-provided list object directly
(entry.indices.append(indices)), which allows external mutation to affect stored
targets; change this to append a defensive copy of the list (e.g., a shallow
copy via list(indices) or indices.copy()) when adding to entry.indices so the
stored data cannot be mutated by the caller later—if stored elements can
themselves be mutable and require isolation, use a deep copy instead.
- Around line 1689-1711: The new add_actuator signature replaces the old
actuator_class/input_indices/output_indices API and will break callers; add a
compatibility shim inside ModelBuilder.add_actuator that detects the legacy
invocation (presence of actuator_class or input_indices/output_indices in kwargs
or positional args), issues a DeprecationWarning referencing the old symbols,
and maps the old parameters to the new ones (map actuator_class →
controller_class, combine input_indices/output_indices into indices or infer
indices as needed, and translate any old per-DOF kwargs) before delegating to
the new logic; ensure the shim preserves existing behavior (including handling
clamping and delay) and only emits the deprecation warning once per process.
- Around line 10349-10379: The loop in ModelBuilder.finalize currently iterates
self.actuator_entries.values(), which processes grouped entries by key and can
reorder actuators versus the original add (insertion) order; fix by tracking and
iterating the actual insertion sequence: add a new list attribute (e.g.,
self.actuator_add_order or self.actuator_entries_order) that append-receives a
reference/tuple for each actuator when created (update the code-path that
creates entries to append there), then in finalize replace for entry in
self.actuator_entries.values(): with a loop over that insertion list so you call
_build_index_array, _stack_args_to_arrays, construct controller/Delay/clamping
and create Actuator in the exact add order; ensure any existing code using
self.actuator_entries still works and remove reliance on dict value ordering.
In `@newton/_src/utils/import_usd.py`:
- Around line 2759-2767: The code currently maps each joint path to a single DOF
index (path_to_dof using builder.joint_qd_start), so multi-DOF joints lose their
extra DOFs; change this to map each joint path to the full list/range of DOF
indices for that joint (e.g., compute start = builder.joint_qd_start[idx] and
count = number of DOFs for that joint—obtainable from builder (e.g.,
builder.joint_qd_count) or by using the next joint start to infer the range) and
build path_to_dofs -> list(range(start, start+count)); then when processing
parsed.target_paths in parse_actuator_prim, gather and flatten all DOF indices
from path_to_dofs (instead of a single index) into dof_indices before calling
add_actuator so actuators target all DOFs of multi-DOF joints (referencing
path_to_dof/path_to_dofs, builder.joint_qd_start, parsed.target_paths,
parse_actuator_prim, add_actuator, and dof_indices).
---
Nitpick comments:
In `@newton/_src/actuators/actuator.py`:
- Around line 160-168: The SHARED_PARAMS attribute is implemented as a
recomputed `@property` but named like a constant; change it to a class variable or
compute/cache it once in the instance to match the ClassVar pattern used by
Delay/Controller. Modify SHARED_PARAMS from a property to either a
ClassVar[set[str]] (if static) or compute the composed set in __init__ (using
self.controller.SHARED_PARAMS, self.delay.SHARED_PARAMS when delay is not None,
and each c.SHARED_PARAMS from self.clamping) and store it on the instance (e.g.,
self.SHARED_PARAMS) so subsequent accesses do not recompute; if dynamic updates
are required, add explicit invalidation or lazy caching instead. Ensure
references to SHARED_PARAMS elsewhere continue to work with the new
class/instance attribute semantics.
In `@newton/_src/actuators/clamping/base.py`:
- Around line 50-73: The modify_forces method docstring is missing SI units for
physical quantities and the wp.array type annotations lack dtype brackets;
update the docstring for modify_forces to state SI units for forces (e.g.,
newtons, N), positions (meters or radians as appropriate), and velocities (m/s
or rad/s), and change the type hints for src_forces, dst_forces, positions,
velocities, and input_indices to use bracketed Warp array dtypes (e.g.,
wp.array[float32] or wp.array[int32] as appropriate) while leaving num_actuators
as int; ensure the docstring units match the semantics used elsewhere in the
Actuator implementation.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 82-85: Update the public docstring for the clamping position-based
API to include SI units for the physical quantities: annotate lookup_angles as
in radians (e.g., lookup_angles [rad]) and lookup_torques as in newton-metres
(e.g., lookup_torques [N·m]) in the Args section of the constructor/function
(search for lookup_angles and lookup_torques in ClampingPositionBased /
clamping_position_based.py) so the docstring clearly states the units.
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 175-183: The update_state method currently only guards next_state
but not current_state; modify ControllerNetLSTM.update_state to also check if
current_state is None (or if self._hidden/self._cell are uninitialized) and
return early if so, to avoid using self._hidden and self._cell when compute()
wasn't called; reference the method name update_state, parameters current_state
and next_state, and the instance attributes self._hidden and self._cell when
adding the guard and returning early.
- Around line 132-173: In compute, avoid fragile object-identity selection using
`target_indices is self._warp_sequential_indices`; instead determine the correct
torch index set by a robust check or explicit flag: either compare index
contents (e.g., element-wise/equality check between `target_indices` and
`self._warp_sequential_indices` or convert to torch and use `torch.equal`) or
add an explicit boolean parameter (e.g., `use_sequential_indices`) passed from
the caller and branch on that; update the selection logic for `torch_target_idx`
(currently choosing between `self._torch_sequential_indices` and
`self._torch_input_indices`) and adjust call sites to provide the flag if you
take that approach.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 37-39: In State.reset(), guard against attributes being None
before calling in-place zeroing: check that self.pos_error_history and
self.vel_history are not None (or use truthy check) before calling .zero_() so
reset() no-ops safely if tensors haven't been initialized; update the reset
method (referenced as reset and attributes pos_error_history, vel_history) to
perform these None checks.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 143-146: The loop contains a redundant None check: since `schemas`
was previously filtered to keys present in `SCHEMA_REGISTRY`,
`SCHEMA_REGISTRY.get(schema_name)` cannot return None; remove the `if entry is
None: continue` branch and either use the direct lookup `entry =
SCHEMA_REGISTRY[schema_name]` or keep `get` but proceed without the dead check
in the loop that iterates over `schemas` (referencing `schemas`,
`SCHEMA_REGISTRY.get`, and the `entry` variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b0f73f95-8be9-48ca-8df9-c382b2ec21df
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
docs/guide/release.rstnewton/__init__.pynewton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/__init__.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/__init__.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/kamino/README.mdnewton/_src/utils/import_usd.pynewton/_src/utils/selection.pynewton/actuators.pynewton/tests/assets/actuator_test.usdanewton/tests/test_actuators.pypyproject.toml
💤 Files with no reviewable changes (2)
- newton/_src/solvers/kamino/README.md
- pyproject.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/_src/actuators/usd_parser.py (1)
168-171:⚠️ Potential issue | 🟡 MinorUse metadata-derived schemas in the missing-controller error message.
This message currently reports
prim.GetAppliedSchemas(), which does not reflect the custom schema tokens you actually parsed from metadata. Reportschemas(and ideally the prim path) instead for accurate debugging.In Pixar USD, does UsdPrim.GetAppliedSchemas() include unregistered custom API schema tokens that are only present in the prim's `apiSchemas` metadata list?🔧 Proposed fix
if controller_class is None: raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" + f"Actuator prim '{prim.GetPath()}' has no controller schema applied " + f"(metadata schemas checked: {schemas})" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 168 - 171, The error raised when controller_class is None uses prim.GetAppliedSchemas(), which can omit custom API schema tokens parsed from metadata; update the ValueError to include the parsed schemas variable (e.g., schemas) and the prim path (e.g., prim.GetPath()) so the message reflects metadata-derived schemas and the prim location. Locate the check around controller_class in usd_parser.py and replace or augment the error text to reference schemas and prim.GetPath() (and keep prim.GetAppliedSchemas() only if helpful) to provide accurate debugging info.
🧹 Nitpick comments (1)
newton/_src/actuators/usd_parser.py (1)
143-146: Remove redundant registry lookup branch.
get_schemas_from_prim()already filters to keys inSCHEMA_REGISTRY, soentry is Nonehere is unreachable in normal flow.♻️ Proposed cleanup
for schema_name in schemas: - entry = SCHEMA_REGISTRY.get(schema_name) - if entry is None: - continue + entry = SCHEMA_REGISTRY[schema_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 143 - 146, The loop over schemas contains an unreachable defensive check for entry = SCHEMA_REGISTRY.get(schema_name) followed by "if entry is None: continue"; since get_schemas_from_prim() already guarantees schema_name is in SCHEMA_REGISTRY, remove that branch and use entry directly inside the loop (or replace it with an assertion like assert entry is not None if you want a sanity check). Update the loop in usd_parser.py to delete the "if entry is None: continue" lines and proceed to use the retrieved entry variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 125-130: The docstring and type hint for parse_actuator_prim are
inconsistent with the code path that raises ValueError when controller schemas
are malformed; update the contract to be explicit: either change the
exception-throwing code in the controller/schema handling (the block that
currently raises ValueError) to return None for invalid actuator definitions, or
update the parse_actuator_prim docstring and return type to indicate it may
raise ValueError on malformed schemas (and add a brief sentence about fail-fast
behavior), and adjust any callers/tests accordingly; reference the
parse_actuator_prim function and the ValueError-raising controller/schema
validation block when making the change.
---
Duplicate comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 168-171: The error raised when controller_class is None uses
prim.GetAppliedSchemas(), which can omit custom API schema tokens parsed from
metadata; update the ValueError to include the parsed schemas variable (e.g.,
schemas) and the prim path (e.g., prim.GetPath()) so the message reflects
metadata-derived schemas and the prim location. Locate the check around
controller_class in usd_parser.py and replace or augment the error text to
reference schemas and prim.GetPath() (and keep prim.GetAppliedSchemas() only if
helpful) to provide accurate debugging info.
---
Nitpick comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 143-146: The loop over schemas contains an unreachable defensive
check for entry = SCHEMA_REGISTRY.get(schema_name) followed by "if entry is
None: continue"; since get_schemas_from_prim() already guarantees schema_name is
in SCHEMA_REGISTRY, remove that branch and use entry directly inside the loop
(or replace it with an assertion like assert entry is not None if you want a
sanity check). Update the loop in usd_parser.py to delete the "if entry is None:
continue" lines and proceed to use the retrieved entry variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8c99773d-01be-4e70-b7b1-02abbf39a391
📒 Files selected for processing (2)
newton/_src/actuators/usd_parser.pynewton/tests/assets/actuator_test.usda
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/assets/actuator_test.usda
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
newton/_src/utils/import_usd.py (1)
2759-2768:⚠️ Potential issue | 🟠 MajorExpand joint targets to all DOFs before adding actuators.
path_to_dofcollapses each joint path to a singlejoint_qd_start, so an actuator targeting a multi-DOF joint only wires its first DOF. SinceModelBuilder.add_actuator()is single-DOF, this needs to fan out the joint path into every DOF index and add one actuator entry per DOF.Proposed fix
- path_to_dof = { - path: builder.joint_qd_start[idx] for path, idx in path_joint_map.items() if idx < len(builder.joint_qd_start) - } + path_to_dofs: dict[str, list[int]] = {} + for path, joint_idx in path_joint_map.items(): + if joint_idx >= len(builder.joint_qd_start): + continue + dof_start = builder.joint_qd_start[joint_idx] + dof_end = ( + builder.joint_qd_start[joint_idx + 1] + if joint_idx + 1 < len(builder.joint_qd_start) + else len(builder.joint_qd) + ) + path_to_dofs[path] = list(range(dof_start, dof_end)) @@ - if parsed.target_path not in path_to_dof: + dof_indices = path_to_dofs.get(parsed.target_path) + if not dof_indices: continue - dof_index = path_to_dof[parsed.target_path] @@ - builder.add_actuator( - parsed.controller_class, - index=dof_index, - clamping=clamping_specs if clamping_specs else None, - delay=delay_val, - **parsed.controller_kwargs, - ) - actuator_count += 1 + for dof_index in dof_indices: + builder.add_actuator( + parsed.controller_class, + index=dof_index, + clamping=clamping_specs if clamping_specs else None, + delay=delay_val, + **parsed.controller_kwargs, + ) + actuator_count += 1Also applies to: 2779-2786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/utils/import_usd.py` around lines 2759 - 2768, path_to_dof currently maps each joint path to a single start index (using builder.joint_qd_start) which causes actuators to wire only the first DOF of multi-DOF joints; change the logic that builds path_to_dof (and the equivalent block at the second occurrence) to expand each joint path into all its DOF indices by using the joint's DOF count (from builder.joint_qd_start and builder.joint_qd_count or equivalent) and produce a list of indices per path, then when handling parsed = parse_actuator_prim(prim) iterate over all indices for parsed.target_path and call ModelBuilder.add_actuator (or the existing actuator-add code) once per DOF so each DOF gets its own actuator entry.newton/_src/actuators/usd_parser.py (2)
171-174:⚠️ Potential issue | 🟡 MinorReport the metadata-derived schemas in the missing-controller error.
GetAppliedSchemas()does not reflect these custom Newton API tokens, so this message points debugging at the wrong evidence. Use theschemaslist you already extracted fromapiSchemasmetadata instead.Proposed fix
- raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" - ) + raise ValueError( + f"Actuator prim '{prim.GetPath()}' has no controller schema applied " + f"(found schemas: {schemas})" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 171 - 174, The ValueError raised when controller_class is None shows prim.GetAppliedSchemas() which omits metadata-derived Newton API tokens; replace that evidence with the already-extracted schemas list (the variable named schemas from apiSchemas metadata) in the error message so the raised ValueError uses schemas (and include prim.path or similar context if desired) instead of prim.GetAppliedSchemas() in the raise inside usd_parser.py where controller_class is checked.
128-133:⚠️ Potential issue | 🟡 MinorDocument that malformed actuator prims raise, not
None.The current docstring reads like every invalid actuator case returns
None, but this function fail-fast raises on malformed controller schemas. Please make that contract explicit so callers know they must handle exceptions separately from “not an actuator” cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 128 - 133, Update the parse_actuator_prim docstring to state that the function returns a ParsedActuator or None (when the prim is not an actuator) but will raise on malformed actuator/controller schemas rather than returning None; mention parse_actuator_prim by name and note callers must handle exceptions raised during schema parsing (e.g., malformed controller schemas) separately from the None "not an actuator" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 141-153: The code incorrectly tests array identity with
"target_indices is input_indices" and can pick the wrong indices; instead
convert the actual target_indices contents to a torch index tensor and use that
for indexing. Modify the block that sets torch_target_idx to compute a torch
tensor from target_indices (e.g., torch.tensor(target_indices.numpy(),
dtype=torch.long, device=self._torch_device)) when target_indices does not match
the cached self._torch_input_indices, falling back to cached
self._torch_input_indices or self._torch_sequential_indices only as an
optimization; then compute pos_error and vel using that computed
torch_target_idx (use target[torch_target_idx] and
current_pos[self._torch_input_indices] / current_vel[self._torch_input_indices]
as appropriate).
---
Duplicate comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 171-174: The ValueError raised when controller_class is None shows
prim.GetAppliedSchemas() which omits metadata-derived Newton API tokens; replace
that evidence with the already-extracted schemas list (the variable named
schemas from apiSchemas metadata) in the error message so the raised ValueError
uses schemas (and include prim.path or similar context if desired) instead of
prim.GetAppliedSchemas() in the raise inside usd_parser.py where
controller_class is checked.
- Around line 128-133: Update the parse_actuator_prim docstring to state that
the function returns a ParsedActuator or None (when the prim is not an actuator)
but will raise on malformed actuator/controller schemas rather than returning
None; mention parse_actuator_prim by name and note callers must handle
exceptions raised during schema parsing (e.g., malformed controller schemas)
separately from the None "not an actuator" case.
In `@newton/_src/utils/import_usd.py`:
- Around line 2759-2768: path_to_dof currently maps each joint path to a single
start index (using builder.joint_qd_start) which causes actuators to wire only
the first DOF of multi-DOF joints; change the logic that builds path_to_dof (and
the equivalent block at the second occurrence) to expand each joint path into
all its DOF indices by using the joint's DOF count (from builder.joint_qd_start
and builder.joint_qd_count or equivalent) and produce a list of indices per
path, then when handling parsed = parse_actuator_prim(prim) iterate over all
indices for parsed.target_path and call ModelBuilder.add_actuator (or the
existing actuator-add code) once per DOF so each DOF gets its own actuator
entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c8ef74a3-60d9-4c8a-81fb-1651a08729c5
📒 Files selected for processing (14)
newton/_src/actuators/actuator.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/_src/sim/builder.pynewton/_src/utils/import_usd.pynewton/tests/test_actuators.py
🚧 Files skipped from review as they are similar to previous changes (8)
- newton/_src/actuators/clamping/base.py
- newton/_src/actuators/clamping/clamping_dc_motor.py
- newton/_src/actuators/clamping/clamping_position_based.py
- newton/_src/actuators/controllers/base.py
- newton/_src/actuators/delay.py
- newton/_src/actuators/actuator.py
- newton/_src/actuators/controllers/controller_net_mlp.py
- newton/_src/sim/builder.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
newton/_src/actuators/clamping/clamping_position_based.py (1)
84-94: Consider validating that lookup torques are positive.The kernel applies symmetric clamping
[-limit, limit]wherelimitcomes fromlookup_torques. If a user provides negative torque values, the clamping behavior would be inverted (e.g.,-limit>+limit). Consider adding validation that alllookup_torquesvalues are non-negative.🛡️ Proposed validation
if len(lookup_angles) != len(lookup_torques): raise ValueError( f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})" ) + # Validate positive torque limits for symmetric clamping + if any(t < 0 for t in lookup_torques) if not isinstance(lookup_torques, wp.array) else False: + raise ValueError("lookup_torques must contain non-negative values for symmetric clamping") 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/_src/actuators/clamping/clamping_position_based.py` around lines 84 - 94, The code does not validate sign of lookup_torques, which are used as symmetric limits in the clamping logic; convert lookup_torques to a wp.array (as done currently) and then check that all entries in lookup_torques are >= 0 (e.g., using numpy or wp array comparison) and raise a ValueError with a clear message if any value is negative; update the constructor in the ClampingPositionBased class (symbols: lookup_torques, lookup_angles, lookup_size, self.lookup_torques) to perform this non-negative check after conversion and before assigning to self.lookup_torques.newton/_src/actuators/usd_parser.py (1)
96-104: Consider warning when relationship has unexpected target count.
get_relationship_targetsilently returnsNonefor both missing relationships and relationships with multiple targets. This could hide USD authoring errors where someone accidentally added multiple targets. A debug-level warning might help diagnose configuration issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 96 - 104, get_relationship_target currently returns None silently when the relationship is missing or has !=1 targets; add a debug-level warning so unexpected multi-target relationships are surfaced. Modify get_relationship_target to obtain a module-level logger (logging.getLogger(__name__)) and when rel is present but len(rel.GetTargets()) != 1, log a debug message including the prim identity (e.g. str(prim.GetPath()) or prim.GetName()) and the relationship name and actual target count before returning None; leave behavior unchanged for missing relationship but you may also log a debug message for that case if desired. Use the existing calls prim.GetRelationship and rel.GetTargets to locate where to insert the logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 84-94: The code does not validate sign of lookup_torques, which
are used as symmetric limits in the clamping logic; convert lookup_torques to a
wp.array (as done currently) and then check that all entries in lookup_torques
are >= 0 (e.g., using numpy or wp array comparison) and raise a ValueError with
a clear message if any value is negative; update the constructor in the
ClampingPositionBased class (symbols: lookup_torques, lookup_angles,
lookup_size, self.lookup_torques) to perform this non-negative check after
conversion and before assigning to self.lookup_torques.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 96-104: get_relationship_target currently returns None silently
when the relationship is missing or has !=1 targets; add a debug-level warning
so unexpected multi-target relationships are surfaced. Modify
get_relationship_target to obtain a module-level logger
(logging.getLogger(__name__)) and when rel is present but len(rel.GetTargets())
!= 1, log a debug message including the prim identity (e.g. str(prim.GetPath())
or prim.GetName()) and the relationship name and actual target count before
returning None; leave behavior unchanged for missing relationship but you may
also log a debug message for that case if desired. Use the existing calls
prim.GetRelationship and rel.GetTargets to locate where to insert the logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2ce5c866-5b1a-44e9-a3e2-7e4ecf768fa0
📒 Files selected for processing (8)
newton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/usd_parser.pynewton/actuators.pynewton/tests/test_actuators.py
✅ Files skipped from review due to trivial changes (1)
- newton/_src/actuators/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/actuators.py
- newton/_src/actuators/clamping/clamping_max_force.py
- newton/tests/test_actuators.py
2 tasks
eric-heiden
reviewed
Apr 15, 2026
eric-heiden
reviewed
Apr 15, 2026
eric-heiden
reviewed
Apr 15, 2026
eric-heiden
reviewed
Apr 15, 2026
eric-heiden
reviewed
Apr 15, 2026
eric-heiden
reviewed
Apr 15, 2026
Member
|
🟡 Bare AGENTS.md requires bracket-syntax Warp array annotations ( Affected locations include:
Please update these to match the dtypes used in the corresponding kernel signatures. |
eric-heiden
reviewed
Apr 15, 2026
jeff-hough
reviewed
Apr 16, 2026
eric-heiden
reviewed
Apr 16, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
newton/_src/actuators/controllers/controller_net_lstm.py (1)
149-151:⚠️ Potential issue | 🟠 MajorUse
target_indicesvalues directly instead of array identity check.
target_indices is input_indicesonly works when the caller passes the exact same Warp array object. If arrays are equal-but-distinct, this will use_torch_sequential_indicesincorrectly.♻️ Proposed fix
- torch_target_idx = ( - self._torch_input_indices if target_indices is input_indices else self._torch_sequential_indices - ) + # Convert target_indices to torch tensor for correct indexing + torch_target_idx = torch.tensor(target_indices.numpy(), dtype=torch.long, device=self._torch_device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 149 - 151, The current selection of torch_target_idx uses an identity check (target_indices is input_indices) which fails for equal-but-distinct arrays; change the branch in controller_net_lstm.py to compare the values/content of target_indices and input_indices (e.g., element-wise equality or numpy.array_equal/torch.equal) rather than object identity, and then assign torch_target_idx to self._torch_input_indices when the arrays are equal by value, otherwise use self._torch_sequential_indices; update the logic around target_indices, input_indices, _torch_input_indices and _torch_sequential_indices accordingly.newton/_src/actuators/usd_parser.py (1)
176-179:⚠️ Potential issue | 🟡 MinorError message shows
GetAppliedSchemas()which won't include custom schemas.Per lines 110-112, custom schemas like
NewtonControllerPDAPIare not returned byGetAppliedSchemas()since they're not registered USD schema types. The error message is misleading for debugging.if controller_class is None: raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" + f"Actuator prim '{prim.GetPath()}' has no controller schema applied (found schemas from metadata: {schemas})" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 176 - 179, The error message is misleading because prim.GetAppliedSchemas() does not include custom/unregistered API schemas; update the ValueError to include both prim.GetAppliedSchemas() and the prim-level metadata that lists applied API schemas (e.g., prim.GetMetadata('apiSchemas') or equivalent) and also include the resolved controller_class name (controller_class) so the message shows both standard applied schemas and any custom API schemas for easier debugging.newton/_src/actuators/controllers/controller_net_mlp.py (1)
137-139:⚠️ Potential issue | 🟠 MajorUse
target_indicesvalues directly instead of array identity check.Same issue as in
ControllerNetLSTM:target_indices is input_indicesonly works when the caller passes the exact same Warp array object.♻️ Proposed fix
- torch_target_idx = ( - self._torch_input_indices if target_indices is input_indices else self._torch_sequential_indices - ) + torch_target_idx = torch.tensor(target_indices.numpy(), dtype=torch.long, device=self._torch_device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_mlp.py` around lines 137 - 139, The code currently selects torch_target_idx using an identity check (target_indices is input_indices) which fails when callers pass equivalent but distinct arrays; change the selection to compare values instead of object identity by checking content equality (e.g., using an element-wise equality helper such as torch.equal/torch.allclose or numpy.array_equal depending on the tensor/array type) between target_indices and input_indices, and then set torch_target_idx to self._torch_input_indices or self._torch_sequential_indices accordingly; update the logic in controller_net_mlp.py where torch_target_idx is computed to use this value-equality check.
🧹 Nitpick comments (1)
newton/_src/actuators/delay.py (1)
89-89: Use bracketed Warp array annotation for_indices.Per coding guidelines, Warp arrays should use bracket syntax with dtype. This also aligns with the reviewer's request for typed array annotations.
- self._indices: wp.array | None = None + self._indices: wp.array[wp.uint32] | None = NoneAs per coding guidelines: "Annotate Warp arrays with bracket syntax (
wp.array[wp.vec3],wp.array2d[float],wp.array[Any]), not the parenthesized form."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/delay.py` at line 89, The instance attribute annotation self._indices currently uses the parenthesized form "wp.array" — update it to the bracketed Warp array syntax with an explicit dtype, e.g. change "self._indices: wp.array | None" to "self._indices: wp.array[Any] | None" (or a more specific dtype such as wp.array[int32] | None if these are integer indices) inside the class where _indices is defined so the annotation follows the required wp.array[...] form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/actuator.py`:
- Line 200: The current check (has_states = current_act_state is not None and
next_act_state is not None) allows stepping a stateful actuator without both
state objects, causing None dereferences in ControllerPID and stalled delay
buffers; modify step() so that for any actuator with a controller state or
nonzero delay (e.g., check self.controller is not None or self.delay > 0 or
whatever stateful flags your Actuator exposes) the code asserts or raises
immediately if either current_act_state or next_act_state is None, and apply the
same fail-fast check in the other state-handling blocks (the sections around the
existing has_states use and the ranges you mentioned) so the compute path never
executes when required state objects are missing.
- Line 125: Update the get_param method signature to use bracketed Warp typing
by changing its return annotation from "wp.array | None" to "wp.array[Any] |
None" in the get_param function; also ensure Any is imported (e.g., from typing
import Any) at the top of the module so the annotation resolves correctly.
- Around line 17-31: The kernel _scatter_add_kernel currently does non-atomic
accumulation (output[idx] = output[idx] + forces[i]) which causes race
conditions; replace both non-atomic writes with wp.atomic_add calls: use
wp.atomic_add(output, idx, forces[i]) for the primary accumulation and, inside
the existing computed_output guard, use wp.atomic_add(computed_output, idx,
computed_forces[i]); ensure you preserve the existing conditional on
computed_output and keep the same argument types (forces, computed_forces,
indices) and kernel signature.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 93-98: The lookup tables self.lookup_angles and
self.lookup_torques are created on Warp's current default device, which can
mismatch the device used later in modify_forces and cause kernel failures;
update modify_forces (or add a set_device(self, device) helper) to check the
target device parameter and migrate these arrays when needed (e.g., compare
array.device to the passed device and recreate or call Warp's device-migration
API to move lookup_angles and lookup_torques onto that device before launching
kernels) so the arrays are always on the same device as the kernel.
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 44-46: The reset method on the State class calls
self.hidden.new_zeros(...) and self.cell.new_zeros(...) unconditionally which
will raise AttributeError when hidden or cell is None; update State.reset to
guard these attributes (e.g., if self.hidden is not None: self.hidden =
self.hidden.new_zeros(self.hidden.shape) and similarly for self.cell) or
alternatively remove None from the type hints if state() guarantees non-None;
modify the reset implementation (referencing the reset method and attributes
hidden and cell) to perform these None-checks so it no longer dereferences None.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 37-39: State.reset() unconditionally calls
self.pos_error_history.zero_() and self.vel_history.zero_() but the attributes
are annotated as optional, causing AttributeError if they are None; update
reset() in the State class to guard these calls (e.g., if self.pos_error_history
is not None: self.pos_error_history.zero_() and similarly for self.vel_history)
or, if invariants guarantee they are never None after state() initialization,
remove the "| None" from the pos_error_history and vel_history type hints and
add an assertion in state()/constructor to enforce non-None.
In `@newton/_src/actuators/controllers/controller_pid.py`:
- Around line 76-83: The resolve_arguments method currently allows negative
integral_max which will cause the integral anti-windup clamp (used by the PID
controller, referenced by wp.clamp in this class) to receive an invalid range;
update resolve_arguments to validate that args.get("integral_max") is not
negative (reject negatives by raising ValueError with a clear message) and keep
the same default math.inf, and add the same validation in the controller's
__init__ (e.g., in ControllerPID.__init__) to guard against callers bypassing
resolve_arguments; ensure both places either coerce invalid values or raise so
integral_max is always >= 0.0.
---
Duplicate comments:
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 149-151: The current selection of torch_target_idx uses an
identity check (target_indices is input_indices) which fails for
equal-but-distinct arrays; change the branch in controller_net_lstm.py to
compare the values/content of target_indices and input_indices (e.g.,
element-wise equality or numpy.array_equal/torch.equal) rather than object
identity, and then assign torch_target_idx to self._torch_input_indices when the
arrays are equal by value, otherwise use self._torch_sequential_indices; update
the logic around target_indices, input_indices, _torch_input_indices and
_torch_sequential_indices accordingly.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 137-139: The code currently selects torch_target_idx using an
identity check (target_indices is input_indices) which fails when callers pass
equivalent but distinct arrays; change the selection to compare values instead
of object identity by checking content equality (e.g., using an element-wise
equality helper such as torch.equal/torch.allclose or numpy.array_equal
depending on the tensor/array type) between target_indices and input_indices,
and then set torch_target_idx to self._torch_input_indices or
self._torch_sequential_indices accordingly; update the logic in
controller_net_mlp.py where torch_target_idx is computed to use this
value-equality check.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 176-179: The error message is misleading because
prim.GetAppliedSchemas() does not include custom/unregistered API schemas;
update the ValueError to include both prim.GetAppliedSchemas() and the
prim-level metadata that lists applied API schemas (e.g.,
prim.GetMetadata('apiSchemas') or equivalent) and also include the resolved
controller_class name (controller_class) so the message shows both standard
applied schemas and any custom API schemas for easier debugging.
---
Nitpick comments:
In `@newton/_src/actuators/delay.py`:
- Line 89: The instance attribute annotation self._indices currently uses the
parenthesized form "wp.array" — update it to the bracketed Warp array syntax
with an explicit dtype, e.g. change "self._indices: wp.array | None" to
"self._indices: wp.array[Any] | None" (or a more specific dtype such as
wp.array[int32] | None if these are integer indices) inside the class where
_indices is defined so the annotation follows the required wp.array[...] form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 85b2ef70-1a33-4243-972c-6282d85f1e4a
📒 Files selected for processing (18)
docs/concepts/actuators.rstdocs/index.rstnewton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/__init__.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/__init__.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/tests/test_actuators.py
✅ Files skipped from review due to trivial changes (6)
- docs/index.rst
- newton/_src/actuators/controllers/init.py
- newton/_src/actuators/clamping/init.py
- newton/_src/actuators/init.py
- docs/concepts/actuators.rst
- newton/_src/actuators/controllers/controller_pd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/actuators/clamping/clamping_max_force.py
Member
There was a problem hiding this comment.
Overall this is fine to merge today — the architecture is solid and the remaining items are edge cases / polish. Let's open follow-up issues for the unresolved threads so they don't get lost, especially the schema-contract items (delay_steps=0, target-type validation, multi-DOF guard) and the standalone-usability gaps (Delay.state() pre-finalize, frc_indices shape check, clamping-order contract).
Test coverage also needs expansion. The existing suite is strong on the happy path but has zero assertRaises — none of the ValueError paths in resolve_arguments / component __init__ are exercised, and the standalone-construction path (building Actuator directly from Warp arrays without ModelBuilder) isn't covered end-to-end. Several of the bugs flagged here (delay_steps=0, pre-finalize .state(), shape mismatches) would have been caught by an error-path sweep and one standalone integration test.
Maybe good to specifically prompt a delta to the new usd schema param ranges and design decisions.
Member
Author
Added a follow up task for testing #2562 |
jeff-hough
reviewed
Apr 23, 2026
jeff-hough
reviewed
Apr 23, 2026
jeff-hough
reviewed
Apr 23, 2026
jeff-hough
reviewed
Apr 23, 2026
jeff-hough
reviewed
Apr 23, 2026
preist-nvidia
previously approved these changes
Apr 23, 2026
Member
preist-nvidia
left a comment
preist-nvidia
left a comment
There was a problem hiding this comment.
Good from my side - can address more in follow-ups and when you are working on lab integration.
jeff-hough
approved these changes
Apr 23, 2026
jeff-hough
left a comment
jeff-hough
left a comment
There was a problem hiding this comment.
All of my comments are addressed, looks good to me!
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Merged
via the queue into
newton-physics:main
with commit Apr 23, 2026
80c0da7
31 of 41 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
Chores