Reuse observed state for delta commands#19
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes delta-mode control in SOArmEmbodiment by caching the most recently observed joint state and reusing it to convert delta actions into absolute joint targets, eliminating an extra get_observation() (and associated serial/camera reads) per control step.
Changes:
- Cache the last observed joint state (
_last_state) during the normal observation cycle and reuse it for delta-to-absolute command conversion. - Clear cached state on
reset()andclose()to avoid stale reuse across episodes/lifecycle boundaries. - Extend tests to cover consecutive delta commands and assert the exact number of observation reads.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/inspect_robots_so101/embodiment.py |
Adds _last_state caching and uses it for delta-mode command conversion; clears cache on reset/close. |
tests/test_embodiment.py |
Updates the fake driver to count observation reads and validates correct behavior across consecutive delta steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self._cfg.joints_are_delta: | ||
| cmd = packing.from_obs_dict(driver.get_observation()) + cmd | ||
| assert self._last_state is not None | ||
| cmd = self._last_state + cmd |
|
Thanks, this is a clean fix. Using the state from the observation the policy actually conditioned on is arguably more correct for delta checkpoints than the old fresh read, since that's what the delta was computed against. The consecutive-delta test pinning the exact I reproduced your validation locally: 81 passed, 100% branch coverage, ruff and mypy clean, and it merges cleanly with your #18. One ask before merge: swap the Also, both of your PRs here are unusually high quality. Claims that reproduce, tests that pin the actual bug, no scope creep. Appreciate it, and I'd be glad to see more. |
Asserts are stripped under python -O, which would turn a missing _last_state into an opaque 'None + cmd' TypeError. Mirror the _require_driver() pattern instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged, thanks again. I pushed the RuntimeError guard swap myself so this didn't have to wait on another round trip; everything else is your work as reviewed. This lands in the next release. Both of your PRs were a pleasure to review, and we'd be glad to have more. |
|
Thanks Jay, I really appreciate the thoughtful reviews and the merges. I reached out to Aris by email earlier because I'm interested in what Robocurve is building and would love to help more directly if there's room for an intern. |
Summary
This removes the extra full
get_observation()call in delta mode, which otherwise reads the serial bus and synchronous cameras twice per step.Closes #2.
Validation
python -m pytest --cov(81 passed, 100% coverage)python -m ruff check .python -m ruff format --check .python -m mypy src/inspect_robots_so101/embodiment.py