perf(base): speed up isskewa, cache identity in trexp/trlog/rodrigues - #216
Open
petercorke wants to merge 1 commit into
Open
perf(base): speed up isskewa, cache identity in trexp/trlog/rodrigues#216petercorke wants to merge 1 commit into
petercorke wants to merge 1 commit into
Conversation
…/rodrigues trexp/trlog don't have one dominant wasteful generic call like the isR/trnorm/tr2adjoint/qqmul/qvmul fixes did (rai-opensource#213/rai-opensource#214/rai-opensource#215) - they're deep call chains (trexp -> isskewa -> vexa -> iszerovec -> unittwist_norm -> rodrigues -> skew -> rt2tr -> ishom -> isR) where cost is spread thin across many small layers. Two targeted, verified wins pulled out of that chain: - isskewa (validity check trexp runs on se(3) input) had the same bug ishom had pre-rai-opensource#213: np.linalg.norm on a small fixed matrix, plus an all(S[-1,:] == 0) array-allocation-and-compare for the bottom row. Fixed the same way, ~1.9x faster standalone. - np.eye(3) was rebuilt from scratch on every call in rodrigues (1x), trexp's V-matrix construction (1x), and trlog (2x), despite only ever being used as a read-only operand in an addition/multiplication that produces a new array. Replaced with a module-level constant _EYE3, used only at call sites where it's provably never mutated or returned directly (an aliasing hazard if it were) - the four zero-motion/zero-rotation early-return `np.eye(N)` calls in rodrigues/trexp are untouched, left as fresh arrays, since they hand the object directly to the caller. isskew (the plain, non-augmented so(n) check) was also tried with the same explicit-arithmetic treatment as isskewa, but did NOT show a reliable win under min-of-repeats benchmarking - the only cost it avoids is np.linalg.norm on an already-cheap `S + S.T`, not enough margin to reliably beat by hand-unrolling. Reverted to the original implementation rather than keep an unproven "fix". Net effect, measured old-vs-new in the same process (min of 9 repeats each, to suppress system jitter after an earlier round of misleading single-run numbers): isskewa ~1.9x, rodrigues ~1.13x, trexp ~1.05x, trlog ~1.10x. Real but modest - see the PR description's "further speedup opportunities" section for what a bigger win here would actually require. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
trexp/trlogare much slower thanrodrigues(~20us and ~17us vs ~7us), but unlike the previous three perf PRs (#213, #214, #215) they don't have one dominant wasteful generic-NumPy call to fix - they're deep call chains (trexp->isskewa->vexa->iszerovec->unittwist_norm->rodrigues->skew->rt2tr->ishom->isR) where cost is spread thin across many small layers, each independently re-validating/re-extracting. This PR pulls out the two safe, verified wins found in that chain; it does not close the full gap torodrigues- see "further speedup opportunities" below.isskewa(the validity checktrexpruns on its se(3) input) had the exact same bugishomhad before perf(base): speed up isR/ishom/ishom2 orthogonality checks #213:np.linalg.norm()on a small fixed-size matrix, plus anall(S[-1,:] == 0)array-allocation-and-compare for checking the bottom row is zero. Fixed the same way (explicit sum-of-squares residual + scalar comparisons). ~1.9x faster standalone.np.eye(3)was rebuilt from scratch on every call inrodrigues(1x),trexp'sV-matrix construction (1x), andtrlog(2x) - despite only ever being used as a read-only operand in an addition/multiplication that produces a new array. Replaced with a module-level constant_EYE3at those call sites only. Not used at the four zero-motion/zero-rotation early-returnnp.eye(N)calls inrodrigues/trexp(e.g.if iszerovec(w): return np.eye(3)) - those hand the array object directly to the caller, and aliasing them to a shared constant would risk a real bug if a caller ever mutated the returned matrix in place. Left as fresh arrays.isskew(the plain, non-augmented so(n) check) was also tried with the same explicit-arithmetic treatment asisskewa, but did not show a reliable win under careful (min-of-repeats) benchmarking - the only cost it avoids isnp.linalg.normon an already-cheapS + S.T, not enough margin to reliably beat by hand-unrolling the indexing. Reverted to the original implementation rather than keep an unproven "fix" - same judgment call as an earlier fully-scalarisRvariant that was tried and discarded for the same reason.Net effect, measured old-vs-new in the same process, min of 9 repeats each (single-run
timeitnumbers were misleading here - see commit message): isskewa ~1.9x, rodrigues ~1.13x, trexp ~1.05x, trlog ~1.10x. Real, but modest compared to #213-#215.Further speedup opportunities (not attempted here)
A bigger win on
trexp/trlogwould mean collapsing the redundantgetvector/re-validation layers in the hot path into a more direct, fused implementation - e.g.trexp(se3_matrix)currently re-extracts and re-validates the same data throughisskewa->vexa->iszerovec->unittwist_normindependently, each with its owngetvectorcall. That's a larger, riskier change: this code has clearly-deliberate handling for numerically delicate edge cases (near-zero rotation, near-pi singularity, caller-suppliedtheta), so a fused rewrite needs much more careful edge-case test coverage than the drop-in fixes here. Not scoped or attempted in this PR.Two smaller, unrelated candidates surfaced but out of scope for this PR:
np.linalg.normis used directly (instead of the project's ownnorm(), which is faster for small vectors per its own docstring) in several other places -iszerovec,isunitvec,qnorm/qunitinquaternions.py, and a few spots ingeom2d.py/geom3d.py/pose3d.py. Worth noting:iszerovecwas checked directly as part of this investigation, and swapping it to the project'snorm()did not show a reliable win either (consistent with the project's own timing script, which showsbase.norm(R6)measuring slower thannp.linalg.norm(R6)for a 6-vector) - so this isn't a guaranteed win either, just an inconsistency worth someone measuring properly before touching.np.eye(N)is rebuilt from scratch in ~20 other places across the codebase (pose2d.py,pose3d.py,transforms2d.py,transformsNd.py, and other unrelated functions intransforms3d.py). None are in thetrexp/trlog/rodriguespath touched here; a broader sweep would need the same operand-vs-return-value aliasing care taken in this PR, function by function.Test plan
pytest tests/- 343 passedisskewaverified against the prior implementation over 3000 randomized trials (skew/non-skew, near-boundary perturbations)rodrigues,trexp,trlog(both matrix and twist-vector return forms) verified bit-identical (max diff 0.0) against reference copies of the prior implementations over 1000-2000 randomized trials each🤖 Generated with Claude Code