Fix sign bugs in Cl(3,0) geometric_product breaking associativity#14
Open
research-developer wants to merge 1 commit into
Open
Conversation
The hardcoded Cayley table in turboquant/clifford.py::geometric_product had sign errors in r1, r2, r3, r12, r13, r23, r123. The full geometric product failed (a * b) * c == a * (b * c) — a foundational axiom of any Clifford / geometric algebra — and computed wrong basis-vector products such as e23 * e123 (was +e1, should be -e1) and e12 * e3 (was -e123, should be +e123). The associativity test in tests/test_clifford.py was previously @pytest.mark.xfail with the comment "Full GP table has a sign error in some terms; RotorQuant uses the sparse GP path which is correct". The corrected Cayley table is derived from scratch by bubble-sort sign tracking of basis-vector concatenations, collapsing e_i^2 = +1. Derivation pinned to module docstring with a worked example for the e23 * e123 entry, and cross-referenced from the new test docstrings. Verified on the standard Cl(3,0) basis [s, e1, e2, e3, e12, e13, e23, e123] with e12 = e1 e2, e13 = e1 e3, e23 = e2 e3, e123 = e1 e2 e3. Tests now passing that previously failed: - TestGeometricProduct::test_associativity (was @xfail, now strict) - TestGeometricProduct::test_associativity_batch (new) - TestGeometricProduct::test_e1_e2_e3_chain_associates (new) - TestGeometricProduct::test_e23_times_e123 (new — direct regression test for the specific table entry called out in the docstring derivation) - TestGeometricProduct::test_e12_times_e3 (new) Tests added that continue to pass (would have on the broken table too, included as belt-and-suspenders so the GP contract is fully covered): - test_scalar_identity_left / test_scalar_identity_right - test_basis_vector_squares (e_i^2 = +1 for grade-1, -1 for grade-2, e123^2 = -1) - test_basis_vector_anticommutation (e_i e_j = -e_j e_i for i != j) Full test suite (tests/test_clifford.py + test_rotorquant.py + test_turboquant.py + test_lloyd_max.py): 94 passed (no regressions). The rotor sandwich, norm preservation, and embed/extract paths that were already passing continue to pass under the corrected table — they were not exercising the broken (a23, b123) / (a13, b123) / (a12, b123) table entries because those require both operands to carry non-zero grade-2 AND grade-3 components, which the sparse RotorQuant path never produces. Discovered during RALL-260 Phase G (3D Clifford-rotor KV-cache quantization) in the RationAll project; that PR vendored a corrected implementation under leading-underscore names rather than depend on the broken upstream. This commit upstreams the fix so downstream consumers can drop the vendor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes sign errors in the full Cl(3,0) geometric product implementation and strengthens the test suite to prevent regressions.
Changes:
- Corrected multiple Cayley-table sign terms in
geometric_productto restore associativity. - Expanded docstrings/comments to document basis ordering and sign derivations.
- Un-xfailed and added several algebraic identity tests (associativity, identities, basis relations).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| turboquant/clifford.py | Updates geometric product component formulas and documents the corrected Cayley-table/sign logic. |
| tests/test_clifford.py | Re-enables associativity test and adds focused regression tests for core Cl(3,0) identities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+44
| The Cayley table below was derived from scratch via bubble-sort sign | ||
| tracking of basis-vector concatenations, collapsing e_i^2 = +1. | ||
| Each term is the sign of moving the right operand's basis vectors | ||
| past the left operand's basis vectors into canonical order. Spot | ||
| check: e23 * e123 = (e2 e3)(e1 e2 e3); moving e1 leftward past | ||
| e2 e3 picks up two sign flips, leaving e1 e2 e3 e2 e3 = -e1 after | ||
| collapsing e_i^2 -> +1, so the (a23, b123) coefficient on r1 must | ||
| be -1 (not +1 as in earlier revisions of this file). |
Comment on lines
+87
to
+90
| def basis(idx): | ||
| v = torch.zeros(8) | ||
| v[idx] = 1.0 | ||
| return v |
Comment on lines
+74
to
+82
| one = torch.tensor([1.0, 0, 0, 0, 0, 0, 0, 0]) | ||
| x = torch.randn(8) | ||
| assert torch.allclose(geometric_product(one, x), x, atol=1e-6) | ||
|
|
||
| def test_scalar_identity_right(self): | ||
| """x * 1 == x for arbitrary multivector x.""" | ||
| torch.manual_seed(42) | ||
| one = torch.tensor([1.0, 0, 0, 0, 0, 0, 0, 0]) | ||
| x = torch.randn(8) |
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
turboquant/clifford.py::geometric_producthad sign errors inr1,r2,r3,r12,r13,r23, andr123. The full geometric product failed(a * b) * c == a * (b * c)— a foundational axiom of any Clifford / geometric algebra — and produced wrong basis-vector products (e.g.e23 * e123returned+e1instead of-e1;e12 * e3returned-e123instead of+e123).The existing
tests/test_clifford.py::TestGeometricProduct::test_associativitywas already marked@pytest.mark.xfailwith the note "Full GP table has a sign error in some terms; RotorQuant uses the sparse GP path which is correct" — so the bug was known but not yet repaired. This PR repairs it.Repro on
main(before the fix)Fix derivation
The corrected Cayley table was derived from scratch via bubble-sort sign tracking of basis-vector concatenations, collapsing
e_i^2 = +1. The derivation is pinned into the module docstring with a worked example for thee23 * e123entry.Spot check (rewritten in the docstring):
e23 * e123 = (e2 e3)(e1 e2 e3). Movee1leftward paste2ande3, picking up two sign flips:e2 e3 e1 e2 e3 -> e1 e2 e3 e2 e3 -> -e1after collapsinge_i^2 -> +1. Therefore the coefficient ona23 * b123inr1must be-1, not+1as in the prior table.The new docstring carries the basis ordering convention
[s, e1, e2, e3, e12, e13, e23, e123]withe12 = e1 e2,e13 = e1 e3,e23 = e2 e3,e123 = e1 e2 e3so the table can be re-derived by inspection.Test plan
test_associativity: previously@xfail, now strict —(a*b)*c == a*(b*c)on random multivectors PASSES.test_associativity_batch: associativity over a batch of 16 random multivectors PASSES.test_e1_e2_e3_chain_associates:(e1 * e2) * e3 == e1 * (e2 * e3) == e123PASSES (was wrong direction onmain).test_e23_times_e123: direct regression test for the table entry called out in the derivation, asserts= -e1PASSES.test_e12_times_e3: asserts= e123PASSES (was-e123onmain).test_scalar_identity_left/test_scalar_identity_right:1 * x == x == x * 1PASSES (already correct onmain, included as a basic GP contract check).test_basis_vector_squares:e_i^2 = +1for grade-1,-1for grade-2 / pseudoscalar PASSES.test_basis_vector_anticommutation:e_i e_j = -e_j e_ifori != jPASSES.tests/test_clifford.py+test_rotorquant.py+test_turboquant.py+test_lloyd_max.py= 94 passed on the fix branch.The rotor-sandwich, norm-preservation, and embed/extract paths in
test_rotorquant.pywere already passing pre-fix because they don't exercise the broken(a23, b123)/(a13, b123)/(a12, b123)table entries — those require both operands to carry non-zero grade-2 AND grade-3 components, which the sparse RotorQuant path never produces. The fix doesn't break them.Discovery context
Found during RALL-260 Phase G (3D Clifford-rotor KV-cache quantization) in the RationAll project. RALL-260 PR #465 vendored a corrected implementation under leading-underscore names (
_geometric_product,_reverse,_rotor_sandwichinrationall/kv_quant_clifford.py) rather than depend on the broken upstream. Upstreaming the fix here lets downstream consumers drop the vendor.🤖 Generated with Claude Code