Skip to content

Fix ur asset - #626

Merged
matafela merged 1 commit into
mainfrom
cj/fix-ur-asset
Sep 15, 2026
Merged

matafela merged 1 commit into
mainfrom
cj/fix-ur-asset

Conversation

@matafela

@matafela matafela commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Description

  • Remove ur5 ee_link position offset.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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 14, 2026 07:21
@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The asset update appears behaviorally safe, but the repository’s focused-test requirement must be satisfied before merging because the new UR5 case does not verify the frame-offset fix.

Fix All in CodexFindings

  1. P2 UR5 Fix Remains Untested
Fix with agent prompt
### Issue 1
tests/sim/motion/solvers/test_ur_solver.py:212-214
The new UR5 case inherits a solver-only FK → IK → FK round trip. Both sides use the same analytic solver, and the test never compares its pose with the URDF's simulated `ee_link`. Removing, retaining, or introducing an end-link offset in the asset would therefore produce the same assertions. This violates the repository requirement that bug fixes include a focused test proving the corrected behavior, so the requirement must be satisfied before merging.

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

  • Keeps UR10 coverage while selecting model-specific URDF paths and joint-name patterns.
  • Adds UR5 FK/IK round-trip cases.
  • The added test does not independently validate the asset-frame correction it is intended to protect.

Reviews (1) · Last reviewed commit: "update"

Comment on lines +212 to +214
class TestUR5Solver(BaseSolverTest):
def setup_method(self) -> None:
self.setup_simulation("cpu", ur_type="ur5")

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 UR5 Fix Remains Untested

The new UR5 case inherits a solver-only FK → IK → FK round trip. Both sides use the same analytic solver, and the test never compares its pose with the URDF's simulated ee_link. Removing, retaining, or introducing an end-link offset in the asset would therefore produce the same assertions. This violates the repository requirement that bug fixes include a focused test proving the corrected behavior, so the requirement must be satisfied before merging.

Context Used: AGENTS.md (source)

Knowledge Base Used: Motion planning and kinematics

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/sim/motion/solvers/test_ur_solver.py
Line: 212-214

Comment:
**UR5 Fix Remains Untested**

The new UR5 case inherits a solver-only FK → IK → FK round trip. Both sides use the same analytic solver, and the test never compares its pose with the URDF's simulated `ee_link`. Removing, retaining, or introducing an end-link offset in the asset would therefore produce the same assertions. This violates the repository requirement that bug fixes include a focused test proving the corrected behavior, so the requirement must be satisfied before merging.

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

**Knowledge Base Used:** [Motion planning and kinematics](https://app.greptile.com/dexforce/-/custom-context/knowledge-base/dexforce/embodichain/-/docs/motion-planning-and-kinematics.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

@Yuan-Xinyi Yuan-Xinyi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@matafela
matafela merged commit 7c99775 into main Sep 15, 2026
6 checks passed
@matafela
matafela deleted the cj/fix-ur-asset branch September 15, 2026 02:18
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