Skip to content

fix(ga): change default dual mode to Iinv+ for correct B*dual(B)=I#544

Draft
utiberious wants to merge 4 commits intopygae:masterfrom
utiberious:fix/issue-514-mv-dual
Draft

fix(ga): change default dual mode to Iinv+ for correct B*dual(B)=I#544
utiberious wants to merge 4 commits intopygae:masterfrom
utiberious:fix/issue-514-mv-dual

Conversation

@utiberious
Copy link
Copy Markdown
Contributor

Summary

Changes the default dual_mode from 'I+' to 'Iinv+' so that B * B.dual() == I holds, which is the conventional definition of the dual in geometric algebra.

With 'I+', dual(B) = B * I, which does not satisfy this identity in general. The correct definition is dual(B) = B * I_inv.

Fixes #514

Changes

  • galgebra/ga.py: Change dual_mode_value default from 'I+' to 'Iinv+'
  • test/test_test.py: Update existing dual mode test, add test_dual_correctness verifying B * B.dual() == I
  • examples/ipython/Smith Sphere.ipynb: Update notebook outputs to match corrected dual

Test plan

  • Unit tests pass
  • Full CI with nbval notebooks passes (Smith Sphere notebook outputs updated)

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@utensil
Copy link
Copy Markdown
Member

utensil commented Mar 30, 2026

Thanks for digging into this. The convention ('Iinv+') is correct and consistent with kingdon's polarity dual.

A few things to address before this could be considered for merge:

  1. The test comment "For any blade B, B * B.dual() should equal I" is wrong. It holds for unit bivectors in 3D Euclidean (where B²=-1) but fails for vectors (where B²=1, so B * dual(B) = -I). The test itself is fine, just the comment needs fixing.

  2. The Smith Sphere notebook formula needs to be updated, not just the outputs. The down() and up() functions compute n = -1 * N.dual() to get the normal vector to a plane. With 'I+' that minus sign was needed to get the correct outward normal. With 'Iinv+' it flips the projection pole from north to south, which is why cells 9 and 14 show denominator changes from |z+1|² to |z-1|² — geometrically wrong results. The fix is n = N.dual() (remove the minus).

That said, this is a global default change and a breaking change for any user calling .dual() without setting a mode explicitly. Given that the existing mode system already lets users opt into 'Iinv+', we'd rather not change the default in 0.6.0. This PR is useful as a reference for how the convention change affects tests and user code, and could be revisited for a later release with a deprecation notice.

@utiberious
Copy link
Copy Markdown
Contributor Author

Good catches, addressed in 2025e17:

  1. Fixed the test comment: now says "For a unit bivector B with B*B = -1" instead of claiming it holds for all blades.

  2. Fixed the Smith Sphere notebook formula: removed the negation in down() and up() (n = N.dual() instead of n = -N.dual()). With Iinv+ the dual already gives the correct outward normal, so the minus was double-negating. The denominators now correctly show |z+1|^2 (north pole projection).

  3. Accepted: agree that changing the global default is too disruptive for 0.6.0. This PR serves as a reference for how the convention change would affect tests and user code. Users can already opt into Iinv+ explicitly via Ga.dual_mode('Iinv+').

@utensil
Copy link
Copy Markdown
Member

utensil commented Mar 30, 2026

Thanks, CI is green.

I'll leave this PR open for demonstrating the change of default dual mode. Also I'm considering to default to this new dual mode for Kingdon compatible interface in #524 .

@utensil utensil added wontfix component: core Ga, Mv, Metric, etc nice features nice features that won't be implemented in near future and removed wontfix labels Mar 30, 2026
Copy link
Copy Markdown
Member

@utensil utensil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this — the math is correct. 'Iinv+' (dual(A) = A·I⁻¹) is the standard Hodge dual per Doran & Lasenby, and issue #514 confirms the bug: with the old 'I+' default, B * B.dual() gives -I instead of I for a unit bivector.

That said, this is a breaking change in default behavior for any code that calls dual() without explicitly setting Ga.dual_mode(), so a few things are needed before this can merge:

1. Docs need updating

doc/module-components.rst still describes the old default in two places:

  • Line 342: method signature shows GA.dual_mode(mode='I+')
  • Line 359: prose says "default for the dual mode is mode='I+'"

Both should be updated to 'Iinv+'.

2. Changelog entry missing

Please add an entry to doc/changelog.rst (following the existing :bug: format) noting the breaking change and migration path, e.g.:

:bug:`514` The default dual mode has changed from `'I+'` to `'Iinv+'` so that
`B * B.dual() == I` holds for unit blades, matching the standard definition
(Doran & Lasenby). To preserve the old behavior, call `Ga.dual_mode('I+')`
at the start of your program.

3. Minor cleanup (optional)

galgebra/primer.py:34 has an explicit Ga.dual_mode('Iinv+') call that was a workaround for this exact bug. With the default fixed it becomes a no-op — worth removing or updating the comment to reflect that it's now just confirming the default.


Note: primer.py already used 'Iinv+' explicitly, which shows this is the right direction. Happy to revisit once the docs and changelog are in place.

Copy link
Copy Markdown
Member

@utensil utensil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three requested items are now addressed:

  • doc/module-components.rst — both the method signature (line 342) and the prose (line 359) updated to 'Iinv+'; prose now correctly says A*I^{-1} is returned
  • doc/changelog.rst — entry added with the :bug:514`` format and migration note
  • galgebra/primer.py — explicit Ga.dual_mode('Iinv+') call kept, comment updated to "Confirms the default" rather than "workaround" framing

The PR is technically complete. Holding off on merge for now — this is a breaking change in default behavior and needs deliberate inclusion into a release rather than a quiet merge. Tracking here for when the time is right.

The default dual_mode was 'I+' which computes self*I. The conventional
Hodge dual in geometric algebra is self*I^{-1}. Changed the default to
'Iinv+' so that for any blade B, B * B.dual() == I holds, matching
standard textbook definitions (Doran & Lasenby, Macdonald).

Fixes #2
The dual fix (issue pygae#514) changes sign conventions in the dual operation,
which propagates through the Smith Sphere calculations. Update saved
outputs to reflect the corrected results.
- Correct test_dual_correctness docstring: B*dual(B)=I holds for unit
  bivectors, not all blades
- Remove negation in down()/up() functions: with Iinv+ mode,
  N.dual() already gives the correct outward normal
- doc/module-components.rst: update method signature and prose to show Iinv+ as default
- doc/changelog.rst: add bug#514 entry noting the breaking change and migration path
- galgebra/primer.py: update comment to reflect Iinv+ is now the default, not a workaround
@utiberious utiberious force-pushed the fix/issue-514-mv-dual branch from 0fb90e8 to c147809 Compare March 31, 2026 11:39
@utensil utensil added this to the 0.8.0 milestone Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: core Ga, Mv, Metric, etc nice features nice features that won't be implemented in near future

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mv.dual() method return wrong result

2 participants