-
Notifications
You must be signed in to change notification settings - Fork 24
Make the atomic-action benchmark suite runnable #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1164,3 +1164,43 @@ def create_tutorial_robot_cfg( | |
| "stop_auto_play_recording", | ||
| "draw_axis_marker", | ||
| ] | ||
|
|
||
|
|
||
| def initialize_benchmark_simulation(args) -> "SimulationManager": | ||
| """Create the tutorial simulation from a benchmark-style namespace. | ||
|
|
||
| Benchmark argument namespaces carry only ``device``/``renderer``; fill the | ||
| remaining launcher fields with tutorial defaults so | ||
| :func:`create_tutorial_simulation` accepts them unchanged. | ||
|
|
||
| Args: | ||
| args: Namespace with optional ``num_envs``/``device``/``renderer``. | ||
|
|
||
| Returns: | ||
| The shared tutorial simulation. | ||
| """ | ||
| namespace = argparse.Namespace( | ||
| num_envs=getattr(args, "num_envs", 1), | ||
| device=getattr(args, "device", "cpu"), | ||
| renderer=getattr(args, "renderer", "auto"), | ||
| headless=True, | ||
| ) | ||
| return create_tutorial_simulation(namespace) | ||
|
|
||
|
|
||
| def compute_pick_close_end_step(compiled=None, invocation_index: int = 0) -> int: | ||
| """Trajectory step where PickUp's hand-close segment ends (lift start). | ||
|
|
||
| Args: | ||
| compiled: Optional compiled engine result; when given, the exact | ||
| ``lift`` segment start of the selected invocation is returned. | ||
| invocation_index: Invocation to inspect within ``compiled``. | ||
|
|
||
| Returns: | ||
| Step index separating the grasp phase from the lift phase. Without a | ||
| compiled result this uses the tutorial defaults | ||
| (``sample_count=120`` + ``hand_interp_steps=12`` + ``settle=0``). | ||
| """ | ||
| if compiled is not None: | ||
| return int(compiled.segment(invocation_index, "lift").start) | ||
| return 120 + 12 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The no-argument fallback returns step 132, but the configured 120-waypoint PickUp trajectory has 65 approach, 12 close, and 43 lift waypoints, so lifting begins at step 77. The pickup, place, and move-held-object benchmarks call this helper without passing their available compiled result. Their dynamics-clearing callback therefore runs too late—or never runs for a 120-waypoint trajectory—making the physical-validation measurements unreliable. Use the compiled Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/tutorials/atomic_action/tutorial_utils.py
Line: 1206
Comment:
**Incorrect pickup phase boundary**
The no-argument fallback returns step 132, but the configured 120-waypoint PickUp trajectory has 65 approach, 12 close, and 43 lift waypoints, so lifting begins at step 77. The pickup, place, and move-held-object benchmarks call this helper without passing their available compiled result. Their dynamics-clearing callback therefore runs too late—or never runs for a 120-waypoint trajectory—making the physical-validation measurements unreliable. Use the compiled `"lift"` segment boundary at these call sites instead of the fixed calculation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new public
initialize_benchmark_simulationfunction leavesargsunannotated. The same omission affectscompiledincompute_pick_close_end_stepand parameters in the new tutorial wrapper APIs. This violates the repository directive that public APIs must be fully annotated, with circular imports guarded byTYPE_CHECKINGwhere needed. This repository requirement must be satisfied before merging.Context Used: CLAUDE.md (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!