Share one MJCF kinematics between the drivers that solve for themselves - #720
Share one MJCF kinematics between the drivers that solve for themselves#720DarksaCY wants to merge 3 commits into
Conversation
`yam._Kinematics` is `MjcfKinematics` in `kinematics.py`, beside the placo solver a URDF driver uses, and it takes the model, the site, the joints and the warm starts. Positronic-Robotics#517 names that file as the home. It gains `max_jump`, which a streamed setpoint is solved under: no seed but the live posture, and no solution further from it than the cap. Without it the class runs the YAM's steps in the YAM's order, and `yam_kinematics_goldens.npz` -- ten joint vectors, the pose each puts `DEFAULT_FRAME` at, the joints IK finds for it, and two targets out of reach -- pins that it answers what the YAM answered, to 1e-9. The goldens were generated from the driver before the move, off hardware, and the fake smoke passes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0e48737e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| from . import RobotStatus, State, command | ||
| from .ik import qpos_from_site_pose | ||
| from .kinematics import MjcfKinematics |
There was a problem hiding this comment.
Rule hidden-dependency violated:
When users install the documented yam extra alone, this new import enters kinematics.py, whose module-level import placo raises before i2rt is loaded; pyproject.toml puts placo only in the hardware extra, so YAM support now has an undeclared dependency on another extra. Move MjcfKinematics to a placo-free module or import placo lazily when Kinematics is instantiated.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3828c2d, and you are right that it was a real break, not a smell: uv run --locked --exact --extra yam python -c 'import positronic.drivers.roboarm.yam' failed with Kinematics support is not installed. Re-run with the hardware extra. My earlier smoke passed only because placo was still in the venv from a previous sync.
MjcfKinematics has its own module, mjcf_kinematics.py, and kinematics.py is byte-identical to main again. The lazy import was the other way out, but PLC0415 refuses an import inside a function and a suppression there would be justifying the coupling rather than removing it. The two files each say why they are two. @vertix — this is the one place I have gone against your section 4, which named kinematics.py as the home; say the word and I will do the lazy import with a noqa instead.
`kinematics.py` imports `placo` at module scope, and only the `hardware` extra carries it. Putting the MJCF solver there gave the `yam` extra an undeclared dependency on another one: `uv run --locked --exact --extra yam` could no longer import the YAM driver at all. It has its own module now, and the two files say why they are two. The site is checked: `mj_name2id` answers -1 for a name the model does not carry, and indexing with it reads the model's last site, so the arm would solve for another frame. `reach_postures` takes the whole target rather than its x and y. Where a seed comes from is the arm's business, and the shared class has no reason to say it is planar. The tolerances are the class's own constants.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3828c2d1af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`reach_postures` is expanded once, so a tuple or a generator serves as well as a list. The docstring also said what an uncapped search is used for rather than what it does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f718f7725
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return geom.Transform3D(self._data.site_xpos[self._site_id].copy(), geom.Rotation.from_quat(quat)) | ||
|
|
||
| def ik( | ||
| self, target: geom.Transform3D, current_q: np.ndarray, max_jump: float | np.ndarray | None = None |
There was a problem hiding this comment.
Rule earn-its-place violated:
max_jump has no production caller in the inspected tree—the YAM always omits it, and only the new test supplies it—so this parameter and its search branch pre-land an API solely for a follow-on driver. Remove it until that driver lands, or land the consumer in this change so an actual use shapes the interface.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| """``max_jump`` is what a streamed setpoint is solved under: no seed but the live posture, and no | ||
| solution further from it than the cap.""" |
There was a problem hiding this comment.
Rule diff-comments violated:
This test depends on capped searches using only the live seed and rejecting solutions beyond the cap; saying that streamed setpoints use this option describes an external caller rather than the behavior under test. Remove the streamed-setpoint narrative and state only the local invariant.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
yam._Kinematicsmoves tokinematics.pyasMjcfKinematics, beside the placo solver a URDF driver uses. It takes the model, the site, the joints and the warm starts, so a second driver that solves against an MJCF can read it. #517 names that file as the home.It gains one thing the YAM did not have:
max_jump, which bounds how far a solution may sit from the joints the arm stands in. A streamed setpoint is solved under it -- no seed but the live posture -- so the arm keeps its shape, and a pose it can reach only in another one comes back as nothing. Without it the class runs the YAM's steps in the YAM's order, wrap included.A no-op for the YAM, and measured as one.
yam_kinematics_goldens.npzholds ten joint vectors drawn inside the model's range, the pose each putsDEFAULT_FRAMEat, the joints IK finds for that pose from a seed 0.15 rad away, and two targets outside the arm's reach. They were generated fromyam._Kinematicsbefore the move, off hardware, andtest_kinematics.pyasserts the shared class answers the same to 1e-9.python -m positronic.drivers.roboarm.yam --fakepasses.This is the first of the changes the Trossen stack recommendations ask for, and #683 rebases on it: its driver has the same class, and gains the 2*pi wrap it is missing.