fix(ik): unify ik_XX/ikine_XX return types and fix IK docs - #652
Merged
Conversation
IKSolution had __iter__ but no __getitem__, so positional indexing (sol[0], sol[1], ...) -- the pattern every existing caller and the old bare-tuple return used -- raised TypeError. Add __getitem__ matching __iter__'s order, and a __repr__ matching the existing custom __str__ instead of the verbose default dataclass repr. Also fixes "Levemberg-Marquadt"/"Marquadt" -> "Levenberg-Marquardt" and "progamming" -> "programming", present throughout this file's docstrings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ik_LM/ik_NR/ik_GN (the fast C++-backed solvers) returned a bare 5-tuple while ikine_LM/ikine_NR/ikine_GN (the pure-Python solvers) already returned IKSolution -- same family of methods, two different return shapes. Wrap the C++ tuple in IKSolution in both ETS.ik_LM/ ik_NR/ik_GN and their RobotKinematics forwarders, and update the return-type annotations and :returns:/:rtype: docstring fields to match (ikine_LM/ikine_NR/ikine_GN/ikine_QP were missing :returns:/ :rtype: entirely -- added those too). Also: - bidirectionally cross-reference each ik_XX with its ikine_XX counterpart (previously only cross-referenced their C++ siblings) - add a loud warning to ik_LM/ik_NR/ik_GN's docstrings that they require the compiled C++ extension and raise RuntimeError without it (e.g. pure-Python builds, Pyodide/JupyterLite) - fix several copy-paste bugs in RobotKinematics.py found while doing this: ik_GN's own "See Also" listed itself instead of ik_LM/ik_NR, ikine_GN's and ikine_QP's listed the wrong solver class entirely (IK_NR instead of IK_GN/IK_QP), and two runblock examples said "ikine_GN"/"ikine_LM" while actually calling ik_NR/ik_LM/ik_GN - fix "Levemberg-Marquadt"/"Marquadt" -> "Levenberg-Marquardt" and "deined" -> "defined" throughout both files Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DHRobot.ik_lm_chan/ik_lm_wampler/ik_lm_sugihara/ik_nr/ik_gn all forwarded to self.ets().<same-name>(...), but ETS has never had methods by these names (only the unified ik_LM/ik_NR/ik_GN, each taking a method= kwarg where relevant) -- every one of these five methods raises AttributeError unconditionally on any call. Their docstrings even have literal ":seealso: TODO" placeholders. No test exercises any of them. Confirmed dead: nothing in tests/ or docs/ references any of the five; the only caller was examples/ik_exp.py (fixed separately). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Called the now-removed dead ets.ik_nr/ik_gn/ik_lm_chan/ik_lm_wampler/ ik_lm_sugihara methods, which never existed on ETS in the first place (same root cause as the DHRobot dead-method removal). Rewired to the real ik_NR/ik_GN/ik_LM(method=...) API with matching parameter names, switched from raw 5-tuple unpacking to IKSolution attribute access (the old unpacking would have silently broken now that these methods return a 6-field IKSolution instead of a 5-tuple), and dropped several entirely unused imports (fknm, swift, spatialgeometry, sys, and unused typing names). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers IKSolution.__getitem__/__repr__, and that ik_LM/ik_NR/ik_GN now return real IKSolution instances rather than a bare tuple. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… verify #379 repro # Conflicts: # tests/test_IK.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #652 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 143 143
Lines 14026 14027 +1
=====================================
- Misses 14026 14027 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ik_LM/ik_NR/ik_GN(fast, C++-backed) returned a bare 5-tuple whileikine_LM/ikine_NR/ikine_GN/ikine_QP(pure-Python) already returnedIKSolution-- same family of methods, two different return shapes. Wrapped the C++ tuple inIKSolution(inETS.pyandRobotKinematics.py's forwarders), and updated return-type annotations and:returns:/:rtype:docs to match.IKSolution.__getitem__(matching__iter__'s order) and a__repr__matching the existing__str__-- previouslysol[0]raisedTypeError, and the default dataclass repr was inconsistent with__str__... warning::toik_LM/ik_NR/ik_GN's docstrings: they require the compiled C++ extension and raiseRuntimeErrorwithout it (pure-Python builds, Pyodide/JupyterLite) -- useikine_LM/ikine_NR/ikine_GNinstead there.ik_XX/ikine_XXpair bidirectionally cross-reference each other (previouslyik_XXonly referenced its C++ siblings, and vice versa).IK.py,RobotKinematics.py,DHRobot.py, and two docs files) plus a few other typos ("deined" -> "defined", "progamming" -> "programming").RobotKinematics.ik_GN's own "See Also" listed itself instead ofik_LM/ik_NR;ikine_GN's andikine_QP's "See Also" referenced the wrong solver class entirely (IK_NRinstead ofIK_GN/IK_QP); two runblock examples said "ikine_GN"/"ikine_LM" in prose while actually callingik_NR/ik_LM/ik_GN.DHRobot.ik_lm_chan/ik_lm_wampler/ik_lm_sugihara/ik_nr/ik_gnall forwarded toself.ets().<same-name>(...), butETShas never had methods by these names -- every one of them raisesAttributeErrorunconditionally, confirmed by direct testing. Their docstrings even had literal:seealso: TODOplaceholders. No test exercised any of them.examples/ik_exp.py, the only caller of those dead ETS-level method names (ets.ik_nr/ets.ik_gn/ets.ik_lm_chan/etc.), to use the realik_NR/ik_GN/ik_LM(method=...)API -- also fixed a 5-tuple unpacking that would have silently broken against the new 6-fieldIKSolution, and dropped several unused imports (fknm,swift,spatialgeometry,sys).Item 3 of a 4-item IK-solver cleanup plan (see
claude-notes/ik-solver-cpp-python-divergence.md). Item 1 (YuMi gripper parent-swap) is PR #649, Item 2 (broken example references) is PR #650.Test plan
IKSolution.__getitem__/__repr__, andik_LM/ik_NR/ik_GNreturn realIKSolutioninstances (isinstancechecks)DHRobotmethods really do raiseAttributeErrorunconditionally, and thatik_exp.pyruns end-to-end after the rewrite-W --keep-going) clean of any new warnings -- only the same 2 pre-existing, unrelatedroboticstoolbox.tools.trchainimport warnings remain🤖 Generated with Claude Code