[Newton] Adds IMU Sensor#5264
Conversation
Greptile SummaryThis PR wires Newton's native
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Imu as isaaclab.sensors.Imu (factory)
participant NewtonImu as isaaclab_newton.sensors.imu.Imu (MISSING)
participant NM as NewtonManager
participant SIMU as newton.sensors.SensorIMU
User->>Imu: Imu(cfg)
Imu->>NewtonImu: __new__ dispatches to Newton backend
NewtonImu-->>User: ImportError (module not found)
Note over NM,SIMU: If implementation existed:
NewtonImu->>NM: request_extended_state_attribute("body_qdd")
NewtonImu->>NM: cl_register_site(body_pattern, xform)
NM->>NM: start_simulation() → builder.request_state_attributes()
NewtonImu->>NM: add_imu_sensor(sites)
NM->>SIMU: "SensorIMU(model, sites=sites, request_state_attributes=False)"
NM-->>NewtonImu: sensor_index
loop Each physics step
NM->>SIMU: sensor.update(state_0)
SIMU-->>NewtonImu: ang_vel_b, lin_acc_b populated
end
Reviews (1): Last reviewed commit: "Fix changelog" | Re-trigger Greptile |
|
|
||
| 0.5.15 (2026-04-13) | ||
| ~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Added | ||
| ^^^^^ | ||
|
|
||
| * Added :class:`~isaaclab_newton.sensors.Imu` sensor wrapping Newton's | ||
| ``SensorIMU``, providing angular velocity and linear acceleration in the | ||
| sensor's body frame. |
There was a problem hiding this comment.
Missing CHANGELOG entry for FrameTransformer fix
The PR includes a second commit (6a00eb6a) that adds _invalidate_initialize_callback to FrameTransformer — a bug fix that ensures sites survive a close()/reinit cycle. This fix is absent from the 0.5.15 CHANGELOG entry, violating the project rule that every change to the source directory must be recorded. It should appear under a Fixed section, e.g.:
Fixed
^^^^^
* Fixed :class:`~isaaclab_newton.sensors.FrameTransformer` failing to re-register
sites after a ``close()``/reinit cycle by adding ``_invalidate_initialize_callback``.There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds Newton-backend IMU sensor support by: (1) creating a Imu/ImuData wrapper around Newton's native SensorIMU, (2) extending NewtonManager with request_extended_state_attribute() / request_extended_contact_attribute() APIs for pre-finalization attribute declaration, (3) wiring Newton IMU types into the base factory classes, and (4) fixing FrameTransformer's _invalidate_initialize_callback to re-register sites on close/reinit cycles. The implementation is clean and well-tested, with a few design points worth considering.
Design Assessment
Design is sound. The approach correctly follows Isaac Lab's established patterns:
- The Newton
Imuclass mirrors the FrameTransformer lifecycle: register sites/attributes in__init__→ resolve in_initialize_impl→ copy in_update_buffers_impl→ clean up in_invalidate_initialize_callback. - Extended attribute request APIs (
request_extended_state_attribute/request_extended_contact_attribute) are a clean, general-purpose mechanism that will benefit future sensors beyond IMU. add_imu_sensor()improves onadd_frame_transform_sensor()by adding acls._model is Nonepre-condition check.- The Warp kernels are minimal and correct — simple masked copies that map 1:1 sensor-to-environment.
Findings
🟡 Warning: request_extended_state_attribute() silently succeeds after model finalization — newton_manager.py:470
If a sensor calls request_extended_state_attribute() after start_simulation() has already run, the attribute is added to _pending_extended_state_attributes but will never be forwarded to the builder (the builder is already finalized and the pending set was cleared at line 637). The call silently succeeds without effect.
The docstring says "Sensors call this during __init__, before model finalization" which documents the expectation, but a defensive guard would catch misuse:
@classmethod
def request_extended_state_attribute(cls, attr: str) -> None:
if cls._model is not None:
raise RuntimeError(
f"request_extended_state_attribute('{attr}') called after model finalization. "
"Extended attributes must be requested before start_simulation()."
)
cls._pending_extended_state_attributes.add(attr)Note: This would need to account for the _invalidate_initialize_callback reinit path, where _model is set but clear() has already reset it to None before the callback fires. Based on the docstring for the invalidation callback ("NewtonManager.close() calls clear() before dispatching STOP"), the guard should be safe — but worth verifying.
The same consideration applies to request_extended_contact_attribute().
🟡 Warning: _invalidate_initialize_callback re-registers body_qdd but not contact attributes — imu.py:183
The IMU invalidation callback re-registers the site and the body_qdd state attribute, which is correct. However, request_extended_contact_attribute() exists as a parallel API. If a future sensor uses both state and contact attributes, the invalidation pattern here sets the precedent. Worth confirming that contact attributes are similarly re-registered where needed — or adding a comment clarifying that IMU only needs state attributes (not contacts).
Currently the IMU only uses body_qdd (state), so this is not a bug — just a note for future-proofing.
🔵 Suggestion: Test coverage could verify the close/reinit cycle — test_imu.py
The test suite is thorough for a new feature PR: initialization, shapes, gravity-at-rest, freefall, reset, and sensor print. The freefall test is a particularly good physics correctness check.
Missing coverage that would be valuable:
- Close/reinit cycle: The FrameTransformer invalidation fix and the IMU invalidation callback are critical for multi-reset workflows. A test that calls
sim.reset()twice (triggering the close → clear → reinit path) would verify the site re-registration logic works end-to-end. - Partial reset:
imu.reset(env_ids=[0])with 2+ envs — verifying only the specified env is zeroed while the other retains its data.
🔵 Suggestion: Version 0.5.14 in extension.toml but CHANGELOG shows 0.5.14 — Minor bookkeeping
The version bump from 0.5.13 → 0.5.14 is consistent between extension.toml and CHANGELOG.rst. No issue here — just noting that the gap is clean (no orphaned 0.5.14 entry exists elsewhere).
Test Coverage
- Feature PR: Coverage — Yes, good baseline
- The test file (
test_imu.py) covers 7 test cases: initialization, data shapes, gravity at rest, angular velocity at rest, reset, freefall acceleration, and sensor print - The gravity-at-rest and freefall tests validate physics correctness with appropriate tolerances (±0.5 m/s²)
- Gaps: Close/reinit cycle test and partial-reset test would strengthen coverage but are not blocking
CI Status
- ✅ pre-commit, Check for Broken Links, Detect Changes, Detect Doc Build Type, labeler
- ⏳ Build Base Docker Image, Build Latest Docs, Build cuRobo Docker Image, license-check (pending)
- ⏭️ Build Multi-Version Docs, Deploy Docs (skipping)
Verdict
Minor fixes needed
Clean, well-structured implementation that follows established Isaac Lab patterns. The extended attribute request APIs are a good general-purpose addition. The test suite covers the important physics behaviors. The main suggestion is adding a defensive guard to request_extended_state_attribute() / request_extended_contact_attribute() to catch post-finalization misuse, and optionally adding a close/reinit integration test. No blocking issues.
| Args: | ||
| attr: State attribute name (must be in ``State.EXTENDED_ATTRIBUTES``). | ||
| """ | ||
| cls._pending_extended_state_attributes.add(attr) |
There was a problem hiding this comment.
🟡 Warning: If this method is called after start_simulation(), the attribute is silently added to the pending set but never forwarded to the builder (it was already finalized and the set cleared). Consider adding a guard:
| cls._pending_extended_state_attributes.add(attr) | |
| if cls._model is not None: | |
| raise RuntimeError( | |
| f"request_extended_state_attribute('{attr}') called after model finalization. " | |
| "Extended attributes must be requested before start_simulation()." | |
| ) | |
| cls._pending_extended_state_attributes.add(attr) |
The _invalidate_initialize_callback path should be safe since NewtonManager.clear() resets _model to None before callbacks fire.
There was a problem hiding this comment.
I think this is fine, users are not expected to call this, and it should be called in order.
AntoineRichard
left a comment
There was a problem hiding this comment.
LGTM, let's wait for CI to come back.
| Args: | ||
| attr: State attribute name (must be in ``State.EXTENDED_ATTRIBUTES``). | ||
| """ | ||
| cls._pending_extended_state_attributes.add(attr) |
There was a problem hiding this comment.
I think this is fine, users are not expected to call this, and it should be called in order.
There was a problem hiding this comment.
Hum, I see this was required at some point, is there a way to make a test that would help catch bugs related to this?
381bd59 to
811abe8
Compare
Description
isaaclab_newton.sensors.imu) that wraps Newton's nativeSensorIMU, providing angular velocity and linear acceleration in the sensor's body frameNewtonManagerwithrequest_state_attribute()/request_contact_attribute()for sensors to declare required extended attributes before model finalizationNewtonManager(add_imu_sensor(), per-step updates in_simulate_physics_only())Imu/ImuDatafactory classes for automatic backend dispatchcl_register_site().