Repository navigation
Conversation
The sensor framework had exactly two types. This adds the backend-agnostic half of a third: the config type, its registry entry, the SensorManager record and cadence, the shared output finalizer, and the get_imu_data accessor. No backend reads it yet. Two conventions differ from the existing types and are documented where they bite: - An IMU has no optical axis, so it measures in its mount frame as authored rather than in the -Z-forward/+Y-up optical frame cameras and LiDARs use. The module docstring's claim is scoped accordingly. - Outputs buffer orientation as xyzw to match rigid_body_quat_w, while both native sensors emit wxyz. finalize_imu_outputs is the single place that converts, and it always copies: a backend typically passes slices of a live native buffer, so casting alone could hand consumers a view that mutates under them. target_kind="world" is rejected rather than given two divergent backend behaviours, and there is deliberately no gravity field - both native sensors already report specific force from the scene's own gravity. backend_cache moves from LidarRecord up to SensorRecord; it is the same "backend-owned per-sensor state" concept for every type. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
scene_manager authors a mount site plus the three native sensors into the MjSpec before compile, mirroring the LiDAR mount site; sensor_setup resolves each sensor's (adr, dim) slice after compile and copies out of sensordata. Classic reads each env's own MjData, Warp slices the batched [nworld, nsensordata] array. mj_rnePostConstraint solves the centripetal term an offset mount sees analytically. Measured on a 0.2 m lever arm spinning at 4 rad/s, the accelerometer reports -3.2 m/s^2, exactly -w^2 r, with no time derivative involved. Site type/size/rgba are render-only for these three sensors - verified that a 500x larger sphere and a box of different dimensions produce byte-identical sensordata. Note mjSENS_TOUCH would use the site's size as a sensing volume, so that is not a general property of sites. The test_lidar_rays fake manager gains collect_imus_due: render_sensors now polls every sensor type, so the stub has to track that surface. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_imus creates one IsaacLab Imu per configured IMU before cloning. The mount resolves to the rigid body that owns the link, not the link Xform, because Imu drives a PhysX rigid-body view. gravity_bias is negated from the scene's own gravity so the two backends agree without ImuSensorConfig carrying a gravity field. The subtle part is refresh_imus. IsaacLab's Imu divides its velocity difference by the dt of the most recent scene.update (imu.py:122 then :218), but the difference itself only advances when .data is touched, and SensorBase.update deliberately does not touch it. Holosoma reads at control rate, so an N-step velocity change was being divided by a one-step dt and the acceleration came out N times too large. Confirmed against free fall, where the true answer is 0: 1 settle step gave ~0, 3 gave 19.78 and 8 gave 68.84, i.e. exactly -(n-1)*g. Refreshing every IMU once per scene.update makes the two cadences equal. This is invisible at rest, where dv is ~0 and the divisor does not matter, which is why earlier at-rest checks passed. It is also present in IsaacLab's own manager_based_rl_env plus its imu_lin_acc observation term, while its test_imu reads every physics step - the cadence assumed here. Note sensor_base.py:191 means debug_vis=True would mask it by forcing the refresh, so a debug flag silently changes sensor values. Cost is ~275 us per IMU per physics step (three PhysX view reads), +20.6% wall clock for four IMUs in one environment at 200 Hz. The per-call cost is fixed, so it amortizes across num_envs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preview 4 exposes neither an IMU nor an accelerometer. Without this, create_sensors would filter the sensor out by isinstance and the failure would surface much later as "has no IMU" from whatever tried to read it. No degraded fallback is offered, unlike the terrain-only LiDAR path: an IMU synthesized from differenced body velocities would quietly omit the centripetal term both native backends measure, so the three backends would disagree for a non-obvious reason. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
imu_angular_velocity, imu_linear_acceleration and imu_orientation, thin reads of get_imu_data following terms/cameras.py. Unlike the camera terms these return flat [N, 3] / [N, 4] tensors, so they are legal in a concatenate=True policy group - the manager's gate is purely obs.ndim != 2, despite its error text naming image terms. The tests pin that acceptance, which is the inverse of what the camera tests pin. imu_orientation is ground truth, not an estimate; a real IMU driver reports no absolute orientation, so a policy trained on it will not have the same input on hardware. Said so in the docstring. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The oracles available without a harness - comparing the gyro and the orientation against robot_root_states - only hold for a zero-offset identity mount and say nothing about the accelerometer under motion. This covers that gap with closed-form expectations: |a| == g settled, |a| == 0 in free fall, a rotated mount reading the identity mount rotated by its mount quaternion, and orientation composing as root (x) mount. The check that earns its keep is the spin scenario. At rest a lever arm is inert, so "offset mount == identity mount" is also satisfied by an implementation that drops the offset entirely - it cannot fail for the bug it looks like it tests. Spinning in free fall, the offset mount must show the centripetal term w^2 r that the zero-offset mount cannot see. Verified by zeroing the mount offset while leaving the expectation in place: the harness fails with "the mount offset looks ignored". Teleports are followed by read-and-discard then read-and-assert. set_actor_states changes velocity instantly, and a differencing backend divides that discontinuity by dt - a 1 m/s jump over a 5 ms step reads as 200 m/s^2. MuJoCo is analytic and indifferent, so the discard is what makes the harness backend-neutral rather than an Isaac Sim workaround. --fps drives a convergence sweep. Over 200-800 Hz the Isaac Sim lever term approaches the analytic value from +5.0% to +3.0%, the residual being the physical alpha x r term; MuJoCo stays flat, as an analytic sensor should. The settled checks are documented as default-rate only: at 1600 Hz a collapsed robot still carries real residual contact acceleration on PhysX, which is the robot moving, not the sensor misreading. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docstring attributed the whole excess over w^2 r to the physical alpha x r term. That overstates what was measured. Comparing two mounts on one body against the centripetal term alone understates the expected difference, since the rigid-body relation is alpha x r + w x (w x r) and the tangential part is real whenever the body's rate is changing. Over an 8x range of physics rates MuJoCo's excess is flat at ~6%, consistent with that term alone given its sensor is analytic and has no dt. Isaac Sim's falls from +5.0% to +3.0%, which is dt-dependent and therefore carries discretization error as well. The two contributions were not separated; alpha was never measured. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mark2000
marked this pull request as draft
October 14, 2026 19:39
The previous note hedged on whether the excess over w^2 r was physical or discretization error. Measuring the difference vector settles it. On MuJoCo with r = 0.25 m and w = [0, 0, 4], the measured difference is [-4.0000, 0.0016, 1.3333] against an analytic w x (w x r) of [-4.0000, 0, 0]: the centripetal part matches to four decimals, and the 1.3333 residual is alpha x r with alpha_y ~ -5.33 rad/s^2. The two terms are perpendicular, so comparing a magnitude against w^2 r alone understates it by sqrt(1 + (alpha r / w^2 r)^2) - 1, which is the +5.4% observed. alpha is nonzero through torque-free precession - an asymmetric articulation spun off a principal axis turns the direction of w while |w| stays fixed - not through any configuration change; joint displacement over the measurement window is ~1e-4 rad. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The check compared a magnitude against w^2 r alone. Since alpha x r is perpendicular to w x (w x r), that leaves a systematic offset of sqrt(1 + (alpha r / w^2 r)^2) - 1 which swamped the error being looked for, and it forced a loose 0.5-2.5x tolerance band to pass at all. Now it compares vectors against alpha x r + w x (w x r), with alpha from a central difference of the measured w. Central differencing is second-order accurate, one order finer than the first-order backward difference a differencing backend uses for acceleration, so the residual reflects the sensor rather than the reference. The result is a convergence signal instead of two percentages: Isaac Sim's residual halves per physics-rate doubling, as a backward difference must, while MuJoCo's is several times smaller at every rate and flattens toward a floor set by the reference. Both are two orders of magnitude below the old comparison's offset. Also drops benchmark figures from the docstrings; they belong in the PR discussion, not in API documentation. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docstring and the Isaac Sim comment both explained why ImuSensorConfig has no gravity field. Documenting a field that does not exist is noise; what matters is that both backends report specific force, which is now stated directly. The accompanying test asserted the same absence. Since extra="forbid" had no other coverage in this file, it is reframed as what it was really exercising -- an unknown field being rejected at construction -- and now covers all three sensor config types rather than one arbitrary field name. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mark2000
marked this pull request as ready for review
October 14, 2026 20:38
ruff, ruff-format and mypy, none of which I could run locally when the code was written - ruff is not installed in either conda env, and the local pre-commit mypy trips on a pydantic syntax error that CI's container does not hit. ruff: give two broad pytest.raises a match pattern (PT011), turn a transform loop into a comprehension (PERF401), split an over-long f-string (E501). ruff-format reflowed five files. mypy: narrow ObservationManager.compute()'s Union before indexing .shape, annotate the _finalize helper's keyword arguments, and parameterize the dict in a parametrized test signature. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found two comments asserting invariants the code does not hold. The preset comment claimed this IMU's gyro "must equal the base_ang_vel obs term exactly". It cannot. MuJoCo solves sensordata inside mj_forward, before the integration that advances the qpos/qvel those obs terms proxy, so a reading trails the root state by one physics step. Measured while spinning the pelvis: the discrepancy tracks |alpha| * dt to three significant figures (0.00269 against a predicted 0.00268 at |alpha| = 5.37). The earlier measurement that motivated "exactly" was taken on a settled robot, where alpha is ~0 and the lag cannot show. Comment and docstring now say "up to one physics step". The second refresh_imus call site was justified by a state-write path that does not exist: forward_kinematics is reachable only under sim.kinematic_playback. Playback writes poses and joint positions without integrating, so nothing sets a velocity, and both backends' IMUs would report a constant +g and zero rate however fast the clip runs. Rather than document that, BaseSimulator now rejects the combination at construction, and the unreachable call site is gone. Also from the same review: - The world-mount CLI test left target="pelvis" set, so the generic SensorMountConfig check fired first and the test passed even with validate_imu's world rejection deleted. It now clears target, and fails as intended when that check is removed. - SensorMountConfig.orientation still documented identity as the optical convention, which ImuSensorConfig contradicts. Scoped per sensor type. - Dropped a SKIP_EXIT_CODE constant the harness advertised but never returned. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Camera mounts on actor 'robot'" read as a statement of fact rather than a rejection. Changed to "cannot mount on" for all three sensor types, since fixing only the new one would leave the trio inconsistent. The two tests that assert on these match the unchanged "use target_kind='robot_link'" clause, so no test needed updating. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's isaacsim job failed the settled scenario: |a| = 10.83 against 9.81 +/- 0.3, and a two-mount difference of 2.43 m/s^2 with |w| of only 0.032 rad/s. The second number is the diagnosis - 2.43 / 0.25 m is 9.7 rad/s^2 of angular acceleration. An unactuated robot collapses and then chatters against the plane: negligible angular velocity, large angular acceleration. It is not at rest, and the scenario assumed it was. Three changes, two of which remove guesses: - The two-mount bound used ANGULAR_ACCEL_ALLOWANCE = 2.0 rad/s^2, a constant written before alpha could be measured. It is now central differenced from the sampled window, so the bound is correct at whatever chatter the engine produces, with a stated margin because central differencing understates the peak of a high-frequency signal. - |a| is now a median over a short window rather than one sample. - REST_ACCEL_TOL goes from 0.3 to 1.0. This is a loosening, deliberately: the check exists to catch gravity absent, doubled or wrong-signed, which miss by many m/s^2, and chatter moves |a| by about +/-0.5. At 0.3 it was a flaky contact detector wearing a gravity check's name. The PASS line also reported the middle sample's |a| while the assertion used the median, so it could print a number outside the tolerance it had just passed. It now reports what was checked. Verified on mujoco at fps 200/1600/2000 and isaacsim at 200/1600. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
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
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.
What this adds
A third mounted-sensor type,
ImuSensorConfig, alongsideCameraSensorConfigandLidarSensorConfig— selectable per key like any other sensor:Readings come from each backend's native IMU: MuJoCo's
accelerometer/gyro/framequaton amount site, and IsaacLab's
Imuon Isaac Sim. Isaac Gym Preview 4 has neither and rejects thesensor at startup rather than silently creating nothing.
Outputs are
linear_acceleration,angular_velocityandorientation, read viaget_imu_data(name, data_type)or as observation terms inmanagers/observation/terms/imu.py.24 files, +1496/-31, in six commits;
no_simis green at each one.The two backends do not measure the same quantity
The most important thing for a reviewer to know, and it is inherent rather than an artefact:
mj_rnePostConstraint->sensordatalinear_acceleration-w^2 rO(dt)angular_velocity/orientationThe divergence is confined to
linear_acceleration; neither other path involvesdt.Quantified by the harness's spin check, which compares the two-mount acceleration difference against
the full rigid-body relation
alpha x r + w x (w x r)as vectors.alphacomes from a centraldifference of the measured
w— second-order accurate, so a finer reference than the first-orderquantity under test. Residual is
‖measured − predicted‖, in m/s² and relative to‖predicted‖:Isaac Sim's error halves per
fpsdoubling — first-order, as a backward difference must be.MuJoCo's is 3-4x smaller at every rate and flattens toward a floor, consistent with being limited by
the
O(dt^2)reference rather than by the sensor.Consequence: cross-backend parity must be asserted on steady state, never on transients.
An Isaac Sim correction
IsaacLab's
Imudivides its velocity difference by thedtof the most recentscene.update(
imu.py:122then:218), but the difference only advances when.datais touched, andSensorBase.updatedoes not touch it. Reading at control rate therefore divided an N-step velocitychange by a one-step
dt. Against free fall, where the true answer is 0, the error was exactly-(n-1)*g: 1 settle step gave ~0, 3 gave 19.78, 8 gave 68.84.sensor_setup.refresh_imusnow refreshes each IMU once perscene.update, making the cadencesequal. Two notes:
manager_based_rl_env.py:182-197with itsimu_lin_accterm, while its
test_imu.pyreads every physics step — the cadence_dtassumes.dv ~ 0makes the divisor irrelevant.Cost is ~275 us per IMU per physics step (three PhysX view reads), +20.6% wall clock for four IMUs
in one environment at 200 Hz. Fixed per call, so it amortizes across
num_envs;update_decimationdoes not reduce it, as that governs only copy-out. Documented onImuSensorConfig.Testing
no_simmujoco(full, incl. live harness)imu_assert.pymujoco / mjwarp 3-env / isaacsimisaacgymtests/simulators/imu_assert.pyasserts closed-form expectations:|a| == gsettled,|a| == 0in free fall, a rotated mount reading the identity mount rotated by its mount quaternion, and
orientation composing as
root (x) mount.The check worth reviewing is the spin scenario. At rest a lever arm is inert, so
"offset mount == identity mount" is also satisfied by an implementation that drops the offset — it
cannot fail for the bug it appears to test. Spinning in free fall, the offset mount must show the
centripetal term the zero-offset mount cannot see. Verified by zeroing the mount offset while
leaving the expectation in place: the harness fails with "the mount offset looks ignored".
Known limitations
reject_imushas unit tests, but the wholeisaacgymsuite skips onthis hardware. CI is the first real exercise.
--fps 1600an unactuatedrobot still carries real residual contact acceleration (
|a| ~ 11.4) on Isaac Sim, so thosechecks fail while free-fall and lever-arm checks pass.
REST_ACCEL_TOLis deliberately notwidened, since its job is catching gravity being wrong.
imu_orientationis ground truth, not an estimate — a real IMU driver reports no absoluteorientation, so a policy trained on it will not have the same input on hardware.
Reviewer notes
gravity_biasis negated fromsim.sim.cfg.gravityrather than hardcoded, so a scenethat changes gravity still reports specific force correctly.
target_kind="world"is rejected rather than given two divergent behaviours — it would read aconstant on MuJoCo, and IsaacLab's
Imuneeds a rigid body to attach to.orientationbuffers asxyzwto matchrigid_body_quat_wwhile both natives emitwxyz.finalize_imu_outputsis the only converter, and it always copies: backends pass slices of a livenative buffer, already float32 on the Warp path, so casting alone would hand consumers a view that
mutates under them. It is a function rather than a
set_bufferoverride because quaternion orderfails silently — the
orientation_wxyzkeyword states the expected order at each call site.refresh_imusruns after bothscene.updatesites, includingforward_kinematicswhere nosim time passes: advancing the history past a state write stops the next sample differencing
across that discontinuity, at the cost of a momentarily meaningless buffered value.
backend_cachemoves fromLidarRecordup toSensorRecord— the same concept for every type.