Skip to content

Add an IMU sensor type backed by each backend's native sensor - #244

Open
Mark2000 wants to merge 14 commits into
mainfrom
feat/imu-sensor-type
Open

Mark2000 wants to merge 14 commits into
mainfrom
feat/imu-sensor-type

Conversation

@Mark2000

@Mark2000 Mark2000 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What this adds

A third mounted-sensor type, ImuSensorConfig, alongside CameraSensorConfig and
LidarSensorConfig — selectable per key like any other sensor:

python -m holosoma.run_sim robot:g1-29dof sensor.imu:g1-pelvis-imu

Readings come from each backend's native IMU: MuJoCo's accelerometer / gyro / framequat on a
mount site, and IsaacLab's Imu on Isaac Sim. Isaac Gym Preview 4 has neither and rejects the
sensor at startup rather than silently creating nothing.

Outputs are linear_acceleration, angular_velocity and orientation, read via
get_imu_data(name, data_type) or as observation terms in managers/observation/terms/imu.py.

24 files, +1496/-31, in six commits; no_sim is 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:

MuJoCo Isaac Sim
source mj_rnePostConstraint -> sensordata backward difference of mount-point velocity
linear_acceleration instantaneous, end of last substep mean over one physics step
centripetal -w^2 r exact, analytic O(dt)
under sub-sampling aliases (spikes in skipped substeps vanish) attenuates (spikes smear)
angular_velocity / orientation instantaneous instantaneous

The divergence is confined to linear_acceleration; neither other path involves dt.

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. alpha comes from a central
difference of the measured w — second-order accurate, so a finer reference than the first-order
quantity under test. Residual is ‖measured − predicted‖, in m/s² and relative to ‖predicted‖:

fps MuJoCo Isaac Sim
200 0.0147 (0.36%) 0.0489 (1.22%)
400 0.0068 (0.16%) 0.0278 (0.70%)
800 0.0041 (0.10%) 0.0141 (0.35%)
1600 0.0033 (0.08%) —
2000 (MuJoCo default) 0.0017 (0.04%) —

Isaac Sim's error halves per fps doubling — 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 Imu divides its velocity difference by the dt of the most recent scene.update
(imu.py:122 then :218), but the difference only advances when .data is touched, and
SensorBase.update does not touch it. Reading at control rate therefore divided an N-step velocity
change 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_imus now refreshes each IMU once per scene.update, making the cadences
equal. Two notes:

  • The same pattern appears in IsaacLab's manager_based_rl_env.py:182-197 with its imu_lin_acc
    term, while its test_imu.py reads every physics step — the cadence _dt assumes.
  • It is invisible at rest, where dv ~ 0 makes 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_decimation does not reduce it, as that governs only copy-out. Documented on
ImuSensorConfig.

Testing

suite result
no_sim 1011 passed, 53 skipped
mujoco (full, incl. live harness) 209 passed, 41 skipped
imu_assert.py mujoco / mjwarp 3-env / isaacsim PASS
isaacgym skipped — not runnable on this hardware

tests/simulators/imu_assert.py asserts 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 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

  • Isaac Gym is unverified. reject_imus has unit tests, but the whole isaacgym suite skips on
    this hardware. CI is the first real exercise.
  • The harness's settled checks are default-physics-rate only. At --fps 1600 an unactuated
    robot still carries real residual contact acceleration (|a| ~ 11.4) on Isaac Sim, so those
    checks fail while free-fall and lever-arm checks pass. REST_ACCEL_TOL is deliberately not
    widened, since its job is catching gravity being wrong.
  • 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.
  • No noise, bias or drift model; no outputs only one backend can produce.

Reviewer notes

  • Isaac Sim's gravity_bias is negated from sim.sim.cfg.gravity rather than hardcoded, so a scene
    that changes gravity still reports specific force correctly.
  • target_kind="world" is rejected rather than given two divergent behaviours — it would read a
    constant on MuJoCo, and IsaacLab's Imu needs a rigid body to attach to.
  • orientation buffers as xyzw to match rigid_body_quat_w while both natives emit wxyz.
    finalize_imu_outputs is the only converter, and it always copies: backends pass slices of a live
    native 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_buffer override because quaternion order
    fails silently — the orientation_wxyz keyword states the expected order at each call site.
  • refresh_imus runs after both scene.update sites, including forward_kinematics where no
    sim 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_cache moves from LidarRecord up to SensorRecord — the same concept for every type.

Mark2000 and others added 7 commits October 14, 2026 11:32
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
Mark2000 marked this pull request as draft October 14, 2026 19:39
Mark2000 and others added 3 commits October 14, 2026 12:40
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
Mark2000 marked this pull request as ready for review October 14, 2026 20:38
Mark2000 and others added 3 commits October 14, 2026 13:43
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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant