Skip to content

Check params shapes when chaining options, and fix Coffee's Twist chain - #206

Open
yichao-liang wants to merge 2 commits into
masterfrom
chain-params-space-check
Open

yichao-liang wants to merge 2 commits into
masterfrom
chain-params-space-check

Conversation

@yichao-liang

Copy link
Copy Markdown
Collaborator

LinearChainParameterizedOption checked that its children share a params space with np.allclose on the bounds, and np.allclose broadcasts.
A child with an empty (0,) space and a child with a (1,) space passed, and so did (1,) and (4,) children with equal scalar bounds.
The chain then took the first child's space, and the other children received params of the wrong shape.
The check now compares the shapes before the bounds, and its message names the chain and both children.

The only chain that relied on it is Coffee's combined Twist, which is built when coffee_combined_move_and_twist_policy is on and coffee_use_pixelated_jug is off.
It chained MoveToTwistJug, which takes no params, with TwistJug, which takes a twist amount when coffee_twist_sampler is on (the default).
Twist took the empty space, its NSRT and its process sampled nothing with null_sampler, and TwistJug's policy fell back to turning the jug to jug_pickable_rot.
The chain now gets a TwistJug with an empty params space, which keeps that behaviour and makes the children agree.
A Twist that took the twist amount could not reach that rotation: the PyBullet policy reads the amount as the target angle, bounded to [-1, 1], while jug_pickable_rot is -π/2 and JugPickable allows 0.1.

The ExoPredicator configs that set coffee_combined_move_and_twist_policy: True also set coffee_use_pixelated_jug: True, which skips the twist options, so none of them built this chain.

Pre-existing problems seen along the way (unchanged here)

  • In the default position control mode, MoveToTwistJug descends onto a rotated jug and knocks it aside, so Twist never reaches the twisting pose.
    In reset mode, the mode the Coffee module docstring uses for twisting runs, Twist turns the jug from 0.52 to -1.67 rad in 36 steps.
  • oracle_process_planning on Coffee with the combined Twist fails the same way on master and here, first with Failed to get pose for object cup0 (Keep each PyBullet world's body ids on its own Objects #202 fixes that).
    With Keep each PyBullet world's body ids on its own Objects #202's pybullet_env.py overlaid, position mode runs out of skeletons, and reset mode crashes during the planner's simulation in PyBulletCoffeeEnv._handle_twisting, where set_joints gets a joint vector of the wrong length.
  • tests/agent_sdk/test_bilevel_sketch_samplers.py::test_refine_and_validate_report_returns_plan fails when its file runs first in a process, on master too; the CFG defaults fixture in Reject wrongly shaped params in ParameterizedOption.ground #205's conftest fixes it.

Test plan

  • Reproduced on master: chains over (0,)/(1,), (1,)/(0,) and (1,)/(4,) children build without error.
    The new test_LinearChainParameterizedOption_params_space_mismatch fails there (three "DID NOT RAISE", and the bounds case raises with no message) and passes here.
  • With the check alone (49dfebc), building Coffee's options with the combined Twist fails with Twist: child TwistJug has params space Box(-1.0, 1.0, (1,), float32), but MoveToTwistJug has Box([], [], (0,), float32).
  • The new tests/ground_truth_models/test_coffee_twist.py runs the Twist process's option on a rotated jug and checks JugPickable: it passes on master and here, and fails with the check alone.
  • In position mode, the Twist process's option follows the same trajectory on master and here (jug angle 1.0137 after 400 steps on both).
  • Chain audit: every env's options built under all 97 combinations of the flags that gate chain construction (Coffee 64, Boil 6, Domino 8, Fan 8, Grow 4, and the defaults of Ants, Balance, Blocks, Circuit, Cover, Float and Laser).
    On master only Coffee's Twist has mismatched children, in the 8 combinations with coffee_use_pixelated_jug=False, coffee_combined_move_and_twist_policy=True and coffee_twist_sampler=True; here all 97 build.
  • Full suite split round-robin by test file over 16 jobs in CI's Ubuntu 24.04 container, at the fix before the Coffee test was added: 2540 passed and 1 failed, the bilevel isolation failure above.
    No other chain tripped the check.
  • CI's 8 pytest shards (pytest-split least_duration, as the workflow runs them) at 5776c72 in the same container: 2542 passed, none failed.
  • Static checks at 5776c72 pass: yapf, isort 5.10.1, docformatter 1.4, mypy and pylint.

🤖 Generated with Claude Code

LinearChainParameterizedOption checked that its children share a params
space with np.allclose on the bounds, which broadcasts: a child with an
empty (0,) space and a child with a (1,) space passed, and so did (1,)
and (4,) children with equal scalar bounds. The chain then silently took
the first child's space, and the other children received params of the
wrong shape.

The check now compares the shapes before the bounds and names the
mismatched children. A parametrized test covers the broadcast cases and
a bounds mismatch.
With coffee_combined_move_and_twist_policy, Twist chains MoveToTwistJug,
which takes no params, with TwistJug, which takes a twist amount when
coffee_twist_sampler is on (the default). The chain passed its check
only because np.allclose broadcast the (0,) and (1,) bounds. Twist took
MoveToTwistJug's empty space, its NSRT and process sampled no params,
and TwistJug's policy fell back to turning the jug to jug_pickable_rot.

The chain now gets a TwistJug with an empty params space, which keeps
that behaviour and makes the children agree. A twist amount could not
replace it: the PyBullet policy reads the amount as the target angle,
bounded to [-1, 1], and jug_pickable_rot is -pi/2.

A test runs the Twist process's option on a rotated jug in the reset
control mode and checks that JugPickable holds afterwards.
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