diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..33ed75d --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,43 @@ +# Changelog + +## Unreleased + +### Changed + +- **Scene seeds are now derived from `instance_id` instead of the instance's + sorted position.** ([#3](https://github.com/robocurve/kitchenbench/issues/3)) + + **Results from before this change are not comparable to results after it.** + Every realized scene is different. Do not compare a policy's score across the + boundary, and do not aggregate runs from either side of it. + + `tasks.py` seeded each `Scene` with `index`, and `derive_seed` hashes only + `(eval_seed, scene_seed, epoch)` — task identity never entered the payload. So + instance *i* of every task drew from a byte-identical PCG64 stream, and 21% of + the suite's continuous setup draws were exact duplicates across tasks + (`stack/jitter_x_cm`, `open_container/container_x_cm` and + `seal_container/lid_offset_cm` were all `+1.0889041282` cm at instance 0). + + Because the tasks shared draws, their scores shared error. Measured over 3000 + eval seeds with a bootstrap CI on the variance ratio, the aggregate score's + variance was inflated 1.5x to 1.9x in the range where the benchmark actually + separates policies (score ~0.35–0.50). The 50 trials carried the weight of + roughly 26–33 independent ones, so confidence intervals computed as if the + trials were independent were about 40% too narrow. + + Seeding from `instance_id` (unique and task-qualified) takes the distinct scene + seeds from 5 to 50 and cross-task duplicate draws to zero. + +- **All task versions bumped** to mark the scene change as machine-readable, not + just documented: the nine tasks on `version="1"` are now `"2"`, and + `scoop_pasta` goes `"2"` -> `"3"`. Every `Scene` stamps `spec.version` into its + metadata, so without this bump two different scene sets would ship under + identical version strings — which is the silent non-comparability this change + exists to prevent. + +### Fixed + +- `test_canonical_instruction_matches_epoch_zero` recomputed the seed from `index` + rather than reading `scene.init_seed`, so it pinned a particular seeding scheme + instead of the invariant it meant to check (displayed instruction == the epoch-0 + realization). It now reads the seed off the `Scene` and is seeding-agnostic. diff --git a/src/kitchenbench/specs.py b/src/kitchenbench/specs.py index 729046f..2b1cf37 100644 --- a/src/kitchenbench/specs.py +++ b/src/kitchenbench/specs.py @@ -34,7 +34,7 @@ class TaskSpec: bimanual: bool max_steps: int instances: tuple[TaskInstance, ...] - version: str = "1" + version: str = "2" description: str = "" @@ -1422,7 +1422,7 @@ def _sort_fixtures( bimanual=True, max_steps=120, instances=_SCOOP_PASTA, - version="2", + version="3", description="Tool-mediated granular handling: manage fill level, then transfer.", ), ) diff --git a/src/kitchenbench/tasks.py b/src/kitchenbench/tasks.py index 878bfe3..c42f7b3 100644 --- a/src/kitchenbench/tasks.py +++ b/src/kitchenbench/tasks.py @@ -14,6 +14,7 @@ from __future__ import annotations import re +import zlib from dataclasses import asdict from typing import Any @@ -40,15 +41,25 @@ def build_scenes(spec: TaskSpec) -> list[Scene]: """Build one Scene per task instance (5 per task).""" scenes: list[Scene] = [] for index, inst in enumerate(spec.instances): + # Derive the scene seed from the instance id, not its sorted position. + # + # instance_id is "/" and is unique across the suite, so this + # decorrelates the per-instance RNG streams. Seeding on `index` gave + # instance i of EVERY task a byte-identical PCG64 stream (derive_seed + # hashes only (eval_seed, scene_seed, epoch) -- task identity never + # entered the payload), so same-shaped distributions at the same sorted + # position realized the SAME number across tasks. + scene_seed = zlib.crc32(inst.instance_id.encode()) & 0xFFFFFFFF # The displayed instruction is the realization of epoch 0 under the default - # eval(seed=0), so it equals the first actual rollout's instruction. - canonical = inst.realize(derive_seed(0, index, 0)) + # eval(seed=0), so it equals the first actual rollout's instruction. It has + # to use the same scene seed the Scene carries, or the two drift apart. + canonical = inst.realize(derive_seed(0, scene_seed, 0)) scenes.append( Scene( id=_slug(inst.instance_id), instruction=canonical.instruction, target=Target(kind=inst.target_kind, spec=dict(inst.static)), - init_seed=index, + init_seed=scene_seed, metadata={ "benchmark": "kitchenbench", "task": spec.key, diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 9f0c8b7..7e5d347 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -57,7 +57,10 @@ def test_scene_metadata_is_json_native() -> None: def test_canonical_instruction_matches_epoch_zero() -> None: spec = SPEC_BY_KEY["pour_pasta"] for index, scene in enumerate(build_scenes(spec)): - expected = spec.instances[index].realize(derive_seed(0, index, 0)).instruction + # Read the seed off the Scene rather than re-deriving it, so this test + # pins the INVARIANT (displayed instruction == epoch-0 realization) and + # not a particular seeding scheme. + expected = spec.instances[index].realize(derive_seed(0, scene.init_seed, 0)).instruction assert scene.instruction == expected