diff --git a/CHANGELOG.md b/CHANGELOG.md index 4deff07..72833b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,17 @@ All notable changes to this project are documented here. The format is based on catch the `ConfigError` raised by `Task`'s epoch validation and surface it through the existing `_resolve_or_exit` pattern, matching how invalid constructor kwargs are handled for config-file components (#47). +- **`DeltaLimitApprover` no longer rejects displacement pose modes whose + rotation deltas are safe to clamp per dimension** (#143). The per-dimension + rotation-repr refusal now fires for absolute pose modes (`eef_abs_pose`, + where clamping an absolute orientation has wraparound and axis-coupling + problems) and, separately, for quaternion deltas in displacement pose modes + (`eef_delta_pose` + `quat_wxyz`/`quat_xyzw`, whose identity is not the zero + vector, so per-dimension clamping distorts the rotation instead of limiting + it). Euler and axis-angle deltas have no such problem and clamp fine, so an + euler-delta embodiment (e.g. BridgeData V2's 7-D xyz+euler deltas) is now + guardrail-ready: `doctor` reports it conformant, and CLI runs keep delta + limiting instead of silently degrading to clamp-only. - **Operator scoring no longer prompts twice for self-confirming embodiments** (#53). On interactive ad-hoc runs, definitive `success` or `failure` termination verdicts are adopted as the operator judgement, announced on the diff --git a/docs/guide/adapters.md b/docs/guide/adapters.md index 82f9996..deb6550 100644 --- a/docs/guide/adapters.md +++ b/docs/guide/adapters.md @@ -112,7 +112,7 @@ constructor hardware-free; connect in `reset()`). | `bounds` | Finite `low`/`high` on every dim. Without them the bounds clamp is skipped and no default delta limit can be derived. | | `dim_labels` | Every dim is named (`("left_j0", ..., "right_gripper")`), uniquely. The agent moves joints by these names. | | `state_alignment` | Absolute-target modes (`joint_pos`, `eef_abs_pose`) declare exactly one `StateSpec` field with `shape == (action_dim,)`: the proprioceptive reference the agent interpolates from. | -| `guardrails` | `DeltaLimitApprover(action_space)` constructs. This catches pose modes with rotation representations that cannot be clamped per dimension (`quat_*`, `axis_angle`, `euler_xyz`; use `none` or `rot6d`). | +| `guardrails` | `DeltaLimitApprover(action_space)` constructs. This catches absolute pose modes (`eef_abs_pose`) whose rotation representation cannot be clamped per dimension (`quat_*`, `axis_angle`, `euler_xyz`; use `none` or `rot6d`), and displacement pose modes (`eef_delta_pose`) carrying a quaternion delta (`quat_wxyz`, `quat_xyzw`; use `none`, `rot6d`, `axis_angle`, or `euler_xyz`). | ### Warnings diff --git a/src/inspect_robots/approver.py b/src/inspect_robots/approver.py index 814487e..eeabf5e 100644 --- a/src/inspect_robots/approver.py +++ b/src/inspect_robots/approver.py @@ -10,13 +10,13 @@ from __future__ import annotations from dataclasses import replace -from typing import Any, Protocol, runtime_checkable +from typing import Any, Protocol, get_args, runtime_checkable import numpy as np import numpy.typing as npt from inspect_robots.errors import SafetyAbort -from inspect_robots.spaces import Box +from inspect_robots.spaces import Box, RotationRepr from inspect_robots.types import Action @@ -80,9 +80,30 @@ def review(self, action: Action, store: dict[str, Any]) -> Action: # change, limited directly). Together these cover every ControlMode literal. _ABSOLUTE_MODES = frozenset({"joint_pos", "eef_abs_pose"}) _POSE_MODES = frozenset({"eef_abs_pose", "eef_delta_pose"}) -# Rotation reps that survive independent per-dimension clamping (same set the -# EnsemblingController accepts for per-dimension averaging). +# Absolute pose modes only: clamping an absolute euler/quat orientation per +# dimension has wraparound and axis-coupling problems, so those reps are +# refused. Displacement pose modes (eef_delta_pose) carry small rotation +# *deltas*, which mostly clamp per dimension like any other bounded +# displacement — except quaternions (see _DISPLACEMENT_POSE_UNSAFE_ROT below). +_ABSOLUTE_POSE_MODES = _ABSOLUTE_MODES & _POSE_MODES +_DISPLACEMENT_POSE_MODES = _POSE_MODES - _ABSOLUTE_POSE_MODES +# Rotation reps that survive independent per-dimension clamping of an absolute +# orientation (same set the EnsemblingController accepts for per-dimension +# averaging). _LIMITABLE_ROT = frozenset({"none", "rot6d"}) +# Displacement pose modes carry rotation *deltas*, whose no-op is not the zero +# vector for these reps (a quaternion identity is (1, 0, 0, 0)). Clamping such +# a delta per dimension toward a symmetric ±max_delta box drags it away from +# identity, and downstream re-normalization can amplify the rotation instead +# of limiting it — the same failure class as clamping an absolute quaternion. +# euler_xyz/axis_angle deltas have no such problem (their no-op is the zero +# vector) and clamp fine. +_DISPLACEMENT_POSE_UNSAFE_ROT = frozenset({"quat_wxyz", "quat_xyzw"}) +# Derived from RotationRepr itself (not hand-listed) so a rep added there +# defaults to the safe side of this message without anyone remembering to +# update it — the refusal set above is still the one source of truth for +# what actually gets rejected. +_DISPLACEMENT_SAFE_ROT = frozenset(get_args(RotationRepr)) - _DISPLACEMENT_POSE_UNSAFE_ROT _LAST_APPROVED_KEY = "delta_limit:last" @@ -103,8 +124,10 @@ class DeltaLimitApprover: explicit ``max_delta`` the limiter adds nothing beyond ``ClampApprover``. Construction never guesses: missing semantics, a needed missing/non-finite - bound without an explicit ``max_delta``, or a pose mode whose rotation - representation cannot be clamped per-dimension all raise ``ValueError``. + bound without an explicit ``max_delta``, an absolute pose mode whose + rotation representation cannot be clamped per-dimension, or a displacement + pose mode carrying a quaternion delta (whose identity is not the zero + vector, so per-dimension clamping distorts it) all raise ``ValueError``. ``NaN`` anywhere in a reviewed action raises [`SafetyAbort`][inspect_robots.errors.SafetyAbort]. A modified action is flagged ``meta["delta_clamped"]``; an unmodified one is returned as the @@ -119,10 +142,21 @@ def __init__(self, action_space: Box, max_delta: float | Any | None = None): "DeltaLimitApprover: action space declares no semantics; the " "limiter cannot tell absolute targets from displacements" ) - if sem.control_mode in _POSE_MODES and sem.rotation_repr not in _LIMITABLE_ROT: + if sem.control_mode in _ABSOLUTE_POSE_MODES and sem.rotation_repr not in _LIMITABLE_ROT: raise ValueError( - f"DeltaLimitApprover: cannot clamp rotation_repr {sem.rotation_repr!r} " - f"per dimension; only {sorted(_LIMITABLE_ROT)} are safe" + f"DeltaLimitApprover: cannot clamp absolute rotation_repr " + f"{sem.rotation_repr!r} per dimension; only {sorted(_LIMITABLE_ROT)} " + f"are safe" + ) + if ( + sem.control_mode in _DISPLACEMENT_POSE_MODES + and sem.rotation_repr in _DISPLACEMENT_POSE_UNSAFE_ROT + ): + raise ValueError( + f"DeltaLimitApprover: cannot clamp displacement rotation_repr " + f"{sem.rotation_repr!r} per dimension; its identity is not at the " + f"origin, so per-dimension clamping distorts it instead of " + f"limiting it; only {sorted(_DISPLACEMENT_SAFE_ROT)} are safe here" ) self._absolute = sem.control_mode in _ABSOLUTE_MODES dim = action_space.dim diff --git a/tests/test_approvers.py b/tests/test_approvers.py index a9c8698..49767fd 100644 --- a/tests/test_approvers.py +++ b/tests/test_approvers.py @@ -13,7 +13,7 @@ from inspect_robots.approver import ChainApprover, ClampApprover, DeltaLimitApprover from inspect_robots.errors import SafetyAbort -from inspect_robots.spaces import ActionSemantics, Box +from inspect_robots.spaces import ActionSemantics, Box, RotationRepr from inspect_robots.types import Action @@ -44,7 +44,9 @@ def test_refuses_missing_semantics() -> None: DeltaLimitApprover(Box(shape=(2,), low=np.zeros(2), high=np.ones(2))) -def test_refuses_pose_mode_with_unaverageable_rotation() -> None: +def test_refuses_absolute_pose_mode_with_unaverageable_rotation() -> None: + # Absolute euler/quat orientations have wraparound + axis coupling, so + # per-dim clamping is unsound and refused. space = Box( shape=(7,), low=np.full(7, -1.0), @@ -55,6 +57,43 @@ def test_refuses_pose_mode_with_unaverageable_rotation() -> None: DeltaLimitApprover(space) +@pytest.mark.parametrize("rotation_repr", ["euler_xyz", "axis_angle"]) +def test_delta_pose_rotation_deltas_clamp_per_dim(rotation_repr: RotationRepr) -> None: + # eef_delta_pose carries small rotation *deltas*, not absolute orientations. + # euler_xyz/axis_angle deltas have a zero-vector no-op, so they clamp per + # dimension like a bounded displacement (#143). Quaternion deltas are the + # exception — see test_refuses_displacement_pose_mode_with_quat_rotation. + # BridgeData V2 shape: 3 xyz + 3 euler deltas + 1 gripper. + space = Box( + shape=(7,), + low=np.full(7, -0.1), + high=np.full(7, 0.1), + semantics=ActionSemantics("eef_delta_pose", rotation_repr=rotation_repr), + ) + approver = DeltaLimitApprover(space, max_delta=0.05) + # A large rotation-delta dim (dpitch, index 4) clamps to the ±max_delta box. + out = approver.review(Action(data=np.array([0.0, 0.0, 0.0, 0.0, 0.5, 0.0, 0.0])), {}) + assert np.isclose(out.data[4], 0.05) + assert out.meta.get("delta_clamped") is True + + +@pytest.mark.parametrize("rotation_repr", ["quat_wxyz", "quat_xyzw"]) +def test_refuses_displacement_pose_mode_with_quat_rotation(rotation_repr: RotationRepr) -> None: + # A quaternion delta's identity is (1, 0, 0, 0), not the zero vector: + # clamping it per dimension toward a symmetric ±max_delta box drags it away + # from identity, and downstream re-normalization can amplify the rotation + # instead of limiting it — same failure class as an absolute quat. + # 3 xyz + 4 quat + 1 gripper = 8 dims. + space = Box( + shape=(8,), + low=np.full(8, -1.0), + high=np.full(8, 1.0), + semantics=ActionSemantics("eef_delta_pose", rotation_repr=rotation_repr), + ) + with pytest.raises(ValueError, match="rotation_repr"): + DeltaLimitApprover(space) + + def test_refuses_derived_default_without_bounds() -> None: for mode in ("joint_pos", "joint_delta"): unbounded = Box(shape=(2,), semantics=ActionSemantics(mode)) diff --git a/tests/test_conformance.py b/tests/test_conformance.py index 5e47bd6..295036f 100644 --- a/tests/test_conformance.py +++ b/tests/test_conformance.py @@ -277,6 +277,7 @@ def test_absolute_mode_needs_exactly_one_aligned_state_field() -> None: def test_unlimitable_rotation_repr_is_an_error() -> None: + # Absolute pose modes still reject unclampable rotation reps. info = _info( space=Box( shape=(7,), @@ -294,6 +295,25 @@ def test_unlimitable_rotation_repr_is_an_error() -> None: assert codes["guardrails"] == "error" +def test_delta_pose_euler_rotation_deltas_are_guardrail_conformant() -> None: + # eef_delta_pose carries rotation deltas, which clamp per dimension, so a + # euler-delta embodiment (e.g. BridgeData V2's 7-D xyz+euler deltas) is + # guardrail-conformant, not rejected (#143). + info = _info( + space=Box( + shape=(7,), + low=np.full(7, -0.1), + high=np.full(7, 0.1), + semantics=ActionSemantics( + "eef_delta_pose", + rotation_repr="euler_xyz", + dim_labels=("dx", "dy", "dz", "droll", "dpitch", "dyaw", "dgrip"), + ), + ), + ) + assert "guardrails" not in _codes(info) + + def test_missing_control_hz_is_a_warning() -> None: info = _good_absolute() silent = EmbodimentInfo(