Skip to content

fix ur solver allocate memory - #604

Open
matafela wants to merge 4 commits into
mainfrom
cj/fix-ur-solver-allocate-too-much-memory
Open

matafela wants to merge 4 commits into
mainfrom
cj/fix-ur-solver-allocate-too-much-memory

Conversation

@matafela

Copy link
Copy Markdown
Collaborator

Description

  • Warp kernel: Added ur_ik_nearest_kernel to generate, validate, and select candidates locally, returning one solution per target. Both paths share analytical branch computation.
  • URSolver: Dispatches by return_all_solutions, removing full candidate allocation and PyTorch selection from the single-solution path while preserving limits, periodic representatives, weights, and failure fallback.
  • Tests: Added 17 CPU/CUDA regression cases covering selection parity, periodic shifts, limits, seeds, ties, and allocation behavior.
  • Benchmark: Compares the new single-solution, legacy selection, and all-solutions paths at 1k/10k/100k targets, reporting warmed timing, throughput, memory, and accuracy.

Fixes # (issue)

#603

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which improves an existing functionality)

Checklist

  • I have run the black . command to format the code base.
  • I have made corresponding changes to the documentation
  • Public API changes are reflected in the API docs (python docs/scripts/check_api_docs.py), if applicable
  • I have added tests that prove my fix is effective or that my feature works
  • Dependencies have been updated, if applicable.

@matafela
matafela requested a review from yuecideng September 10, 2026 03:07
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge.

Fix All in CodexFindings

  1. P2 Annotate variadic keyword values
Fix with agent prompt
### Issue 1
embodichain/lab/sim/motion/solvers/ur_solver.py:undefined-153
The updated public `get_ik` signature leaves `**kwargs` untyped, preventing type checkers and generated API information from determining the accepted keyword-value type.

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!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Extracts shared analytical branch generation for both Warp kernels.
  • Dispatches URSolver.get_ik based on return_all_solutions.
  • Adds regression tests, API documentation, and a solver benchmark.

Reviews (4) · Last reviewed commit: "merge main"

"""Compute target joint positions using OPW inverse kinematics.
) -> tuple[torch.Tensor, torch.Tensor]:
"""Compute target joint positions using UR inverse kinematics.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Annotate variadic keyword values

The updated public get_ik signature leaves **kwargs untyped, preventing type checkers and generated API information from determining the accepted keyword-value type.

Context Used: CLAUDE.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/lab/sim/motion/solvers/ur_solver.py
Line: 148

Comment:
**Annotate variadic keyword values**

The updated public `get_ik` signature leaves `**kwargs` untyped, preventing type checkers and generated API information from determining the accepted keyword-value type.

**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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!

Fix in Codex Fix in Claude Code

Comment thread embodichain/compute/kinematics/_warp/ur.py
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.

2 participants