refactor(ets): extract ET/ETS into own package, add rigorous dh/mdh split()#553
Merged
Conversation
…merge()/split() bugs BaseET/ET/ET2 previously built a compiled C++ struct for every ET, including ET2 (pure Python, 2D) — calling the fast path directly on an ET2's handle silently read out-of-bounds memory since it assumes a 4x4 SE(3) buffer, not ET2's 3x3 SE(2). ET2 now has no compiled handle at all (_accel_init/_accel_update are no-op hooks on BaseET, real on ET), making that class of bug structurally impossible rather than merely unexercised. Along the way: BaseET.__init__ now sets axis_func/flip/jindex/qlim before eta (the eta setter needs axis_func to recompute _T, and reassigning eta post-construction — as ETS.merge() does — silently left the transform matrix stale otherwise, since eta's setter never recomputed _T or synced the C struct). ETS.merge() also had `e1.type`/`e2.type`, referencing an attribute that doesn't exist on ET at all. ETS.split() had also picked up an ETS2-flavoured `split(self, index)` override on the 3D ETS class that shadowed BaseETS's real no-arg segment-splitting split(), breaking Robot() construction from an ETS string. Restored the correct split() on both ETS and ETS2. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ratic tol IKSolver.tol bounds the quadratic weighted angle-axis residual E, not linear-scale pose error directly — with the default tol=1e-6, actual pose error is only guaranteed to be on the order of sqrt(2*tol) ≈ 1.4e-3, looser than the places=4 (~5e-5) checks in test_ikine_LM/test_ikine assumed. Confirmed concretely: a solve with residual=2.87e-8 (well under tol) still had a raw pose difference of 3.27e-4. Not a resurgence of the stale-residual bug fixed on bug/ik-gn-stale-residual — that fix is intact. test_DHRobot.py::test_ikine_LM additionally had no pinned seed, so it was genuinely non-deterministic between runs. Pass tol=1e-10 in both tests (comfortably bounds pose error under places=4's threshold) and pin seed=0 for reproducibility. Document the quadratic/linear relationship on IKSolver's `tol` docstring (and its four solver subclasses) so this isn't rediscovered from scratch next time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ET.axis bundled two concepts into one string ("Rx"/"tx"/"SE3"): the
rotation/translation/special type, and which Cartesian axis. Split these:
- `kind` is the new home for today's `axis` semantics (full string).
- `axis` becomes a permanent deprecated alias for `kind` (warns since
1.4.0) - it is never repurposed to mean the x/y/z-only concept, so it
can never silently mean two different things across versions.
- `ax` is new: returns "x"/"y"/"z" for an elementary transform, None for
ET2's rotation (its axis string is just "R", no letter) and for
compound/arbitrary SE3/SE2 transforms.
Migrated all internal `.axis` call sites (ET.py, ETS.py, fknm.py's
pure-Python Jacobian fallback, RobotPlot.py) to `.kind`/`.ax` so the
library itself never triggers its own deprecation warning.
DHFactor.py/DHFactor2.py are excluded (WIP, being rewritten separately).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`eta` (η) is the name used in the original Elementary Transform Sequence paper (Haviland & Corke) and means nothing to a new reader. `param` is its replacement: - `param` is the new home for the transform-constant property/setter, keeping all the centralized static-transform logic added earlier this session (axis_func validation, `_T` recompute, `_accel_update()` sync). - `eta` becomes a permanent deprecated property alias (getter/setter both warn since 1.4.0, forward to `param`). - The `eta` constructor keyword is similarly renamed to `param` on `BaseET.__init__` and every factory classmethod (Rx/Ry/Rz/tx/ty/tz/SE3 on ET; R/tx/ty/SE2 on ET2), with a keyword-only `eta=` fallback (via a shared `_resolve_param()` helper) so existing `eta=...` calls keep working, warning instead of breaking. Migrated internal call sites (ETS.py's merge()/__str__, Robot.py, PoERobot.py) to `.param`/`param=`. DHFactor.py excluded (WIP, out of scope, as agreed for the axis->kind rename too). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ing)
Constructing a joint currently means either the parameterless factory
(auto-numbered) or a separate `jindex=` int kwarg. `param` now also
accepts a descriptive string - `ET.Rx("theta2")`, `ET.Rx("-q(3)")` - from
which jindex and flip are parsed:
- A leading '-' sets flip (and is stripped); a leading '+' is stripped
and ignored.
- The first run of digits anywhere in what remains becomes the jindex,
via `_parse_joint_descriptor()` - this treats "theta2", "q2", "q(3)"
and "θ_3" the same regardless of how the index is set off from the
rest of the name.
- No digit found (e.g. "theta") falls through to ETS's existing
auto-numbering for unassigned joints - old behaviour maintained.
- A string that parses as a plain number (e.g. "1.5") is a static value,
not a joint descriptor.
- `jindex=`/`flip=` given explicitly alongside a string descriptor raises
ValueError - the two forms are mutually exclusive, not silently merged.
- The original string is remembered (`_joint_name`) and preferred by both
ET.__str__ and ETS.__str__ (which has its own, independent
joint-formatting logic) over the generic "q2"/"-q2".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…plit IK.py's solvers are typed against ETS but only use a small FK/Jacobian surface (n, qlim, jindices, joints(), eval(), jacob0(), jacobm()). RobotProto.py already has precedent for this (KinematicsProtocol, RobotProto) - a matching IKProtocol would make the real dependency explicit instead of overstating it as "needs a whole ETS". Deferred until after the /ets package split, since the import surface IK.py needs from the new location is the same shape this would formalize. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…o split yet) ET/ET2/ETS/ETS2 and their C++ acceleration are the elementary transform sequence representation that Robot/DHRobot/PoERobot are built from, not robot models themselves - a genuinely different, more foundational layer than the rest of robot/. The C++ build already reflects this split cleanly (the _fknm_c CMake target - methods.cpp/ik.cpp/linalg.cpp/ fknm_nb.cpp/structs.h/Eigen - never overlapped with _frne_c's dynamics files), the Python package structure just hadn't caught up. This phase is a pure move, no logic change: ET.py/ETS.py/fknm.py and the _fknm_c source files relocate to src/roboticstoolbox/ets/, CMakeLists.txt updated to match, and every internal import path updated (~20 files: models/, robot/Link.py, robot/BaseRobot.py, robot/Robot.py, etc., plus tests/test_ET.py, tests/test_Robot.py, tests/test_fknm_fallback.py's sys.modules-based patch workaround). Top-level `from roboticstoolbox import ET`/`ETS` is unaffected - only the internal .robot.ET/.robot.ETS import paths moved, and those get a clean break (no deprecated shim) per this being an internal reorg, not a public rename. Also moved: the previously-dead structs.cpp (commented-out, not part of any CMake target, but #includes structs.h which just moved) and split cpp-extensions/README.md into two - the ets/ copy for fknm, a new short one in robot/ for frne - since they now live in different directories. DHFactor.py/DHFactor2.py (WIP) untouched, per earlier agreement - they import ET/ETS via the top-level roboticstoolbox re-export and keep working regardless of where the underlying files live. IK.py stays in robot/ and just updates its ETS import path. Verified: full test suite green (609 passed) against the existing compiled _fknm_c/_frne_c extensions - no C++ code changed, only source file locations and CMakeLists.txt paths, so a from-scratch rebuild wasn't exercised in this session (scikit-build-core unavailable in this environment). The updated CMakeLists.txt paths should be verified with a real build (e.g. in CI) before merging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nctions ET2 (2D) previously shared a file with its 3D counterpart and their common base class, which blocked exposing Rx/Ry/Rz/tx/ty/tz etc as bare importable functions (tx/ty would collide between the two). Splits into: - _ET.py: BaseET only (private/shared - can't import either concrete subclass without a cycle, since both depend on it). - ET.py: the ET class + Rx/Ry/Rz/tx/ty/tz/SE3 as module-level names, so `from roboticstoolbox.ets.ET import *` pulls in exactly the 3D set. - ET2.py: the ET2 class + R/tx/ty/SE2 as module-level names, same idea for 2D. tx/ty deliberately mean something different here than in ET.py - wildcard-importing both into one namespace isn't supported, by design. isinstance(self, ET)/isinstance(self, ET2) inside BaseET's shared __str__/__repr__ (which can't import either concrete subclass) are replaced: __str__'s 3D-vs-2D compound-transform formatting now checks `self.kind == "SE3"`/`"SE2"` directly (already guaranteed at that point - no import needed, more robust than isinstance against subclassing). __repr__'s class-name label now uses `self.__class__.__name__` (idiomatic for a display label, and more correct for any future subclass than a hardcoded "ET"/"ET2" ternary). One landmine hit and fixed while writing this: `SE3 = ET.SE3` / `SE2 = ET2.SE2` (the free-function aliases) would shadow the imported spatialmath SE3/SE2 classes that the classmethod bodies reference by name at call time - fixed by importing them as SE3T/SE2T instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same treatment as ET/ET2/_ET (previous commit), for the sequence classes: - _ETS.py: BaseETS only (private/shared - both ETS and ETS2 depend on it, so it can't depend on either without a cycle). - ETS.py: the ETS class. - ETS2.py: the ETS2 class (imports ET2 directly from ET2.py - safe, one-directional, since ET2.py never depends on ETS2.py at load time). isinstance(self, ETS) inside BaseETS.plot()/teach() (which can't import the concrete ETS without a cycle) is fixed the same way those methods already handle Robot/Robot2: a function-local, call-time-deferred import of ETS from roboticstoolbox.ets.ETS, added alongside the existing local Robot/Robot2 import. Mechanical split of a ~2900-line file done by slicing along the existing class boundaries (BaseETS/ETS/ETS2 were already cleanly delineated) rather than hand-transcribing; each of the three new files keeps the full original import block rather than a hand-traced minimal set, trading a few unused imports per file for confidence against missing something across this much code. Verified: full test suite green (610 passed), including test_plot/ test_teach on both ETS and ETS2 specifically (the isinstance fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests
`ETS.split("dh"/"mdh")` (added mid-flight on this branch) previously used
a heuristic that only absorbed one adjacent constant either side of a
joint and never validated the rest of a segment. Replaced with a real
per-segment parse against the DH/MDH 4-slot template (Rz/tz/tx/Rx or
tx/Rx/Rz/tz), where exactly one of the two z-axis slots is the joint,
the rest are optional but must appear in template order, and content
that can't be accounted for raises ValueError - via a shared
`_split_convention` helper parametrized by template/joint-slots/which
end is exposed vs. folded into the boundary segment.
split()'s return shape changed twice in discussion and settled on a
single flat list `[base, *segments, gripper]` (always length n_joints+2,
base/gripper are an empty ETS - never None - when the method has no
concept of one), enabling `base, *segments, gripper = ets.split(method)`
unpacking and a clean round-trip via `sum(result, ETS())`.
The "first"/"last" methods were pure duplicated logic between ETS.py and
ETS2.py (no DH-specific behaviour), so moved to BaseETS.split() in
_ETS.py; ETS now only overrides for "dh"/"mdh", delegating everything
else to super(). ETS2 inherits it unchanged, gaining base/gripper support
and a correct "unknown split method" error for "dh"/"mdh" it doesn't
support, with zero DH-specific code needed in the 2D class.
Consequence caught by actually running Robot construction, not just
tests: Robot.__init__/Robot2.__init__ built one Link per element of
`arg.split()` directly - the old flat-list contract. The new fixed-shape
list broke this outright (spurious empty base Link, crashed jindex
bookkeeping). Fixed both to unpack base/gripper and only append gripper
as an extra static link when non-empty, restoring original behaviour.
Also added `BaseET.__radd__`/`BaseETS.__radd__` (start value of 0 treated
as identity) so `sum([...])` works on ET/ET2/ETS/ETS2 without an explicit
start, matching `+`/`__mul__` composition already there.
test_ETS.py/test_ETS2.py switched to `from roboticstoolbox.ets.ET(2)
import *` instead of the `rtb.ET./rtb.ET2.` prefix on every call (ET/ET2
already export a curated __all__, so this is clean) - each file is
single-domain so there's no tx/ty cross-collision. Caught two real
hazards by running the suite rather than trusting the mechanical strip:
local variables shadowing the same-named free function within a function
scope (`tx = tx(1.543)` - Python's scoping makes the whole function's
`tx` local, breaking the RHS reference; fixed by renaming the locals to
t_x/t_y/t_z) and `SE3`/`SE2` meaning two different things (ET's
constant-transform-ET constructor vs. spatialmath's pose class) - resolved
by importing ET/ET2's wildcard names before the spatialmath import so the
spatialmath one wins by default, with the handful of genuine
ET-constructor call sites reverted to explicit `rtb.ET.SE3(...)`.
Verified: full suite green throughout (620 passed, 72 skipped).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
ET/ETS(and 2D counterpartsET2/ETS2) out ofrobot/into a newets/package, split intoET.py/ET2.py/_ET.pyandETS.py/ETS2.py/_ETS.py(shared base classes in the underscore-prefixed files).Rx/Ry/Rz/tx/ty/tz/SE3(and 2D equivalents) as free functions alongside the existingET/ET2classmethods, for terser ETS construction.ET.axis→kind, addsaxfor the x/y/z-only concept;eta→param(with a deprecated alias); accepts a string joint descriptor (ET.Rx("theta2")) for jindex/flip parsing.ETS.split("dh"/"mdh"): real per-segment validation against the DH/MDH 4-slot template instead of a heuristic, raisingValueErroron non-conforming content.split()now returns a single flat[base, *segments, gripper]list (base/gripper always an empty ETS, neverNone) for all four methods (first/last/dh/mdh), unpackable asbase, *segments, gripper = ets.split(method)."first"/"last"(pure structural, no DH-specific logic) moved toBaseETSsoETS2inherits them for free instead of duplicating the code.Robot/Robot2construction-from-ETS updated for the newsplit()contract (previously would have crashed - spurious empty base link, broken jindex bookkeeping).ET/ET2/ETS/ETS2all get__radd__sosum([...])works without an explicit start value, matching existing+/*composition.test_ETS.py/test_ETS2.pyswitched fromrtb.ET./rtb.ET2.prefixes tofrom roboticstoolbox.ets.ET(2) import *, since each file is single-domain (no 3D/2D name clash).ikinetests.Test plan
split("dh")/split("mdh")against hand-built DH/MDH chains, including error paths (malformed segment, wrong joint axis, unknown method).Robot(ets)/Robot2(ets)construction across no-trailing-constant, trailing-constant, and leading-constant cases.