Skip to content

Add option to skip SO2 re-normalization - #559

Open
DmitriyKorchemkin wants to merge 1 commit into
strasdat:mainfrom
DmitriyKorchemkin:skip-so2-normalization
Open

Add option to skip SO2 re-normalization#559
DmitriyKorchemkin wants to merge 1 commit into
strasdat:mainfrom
DmitriyKorchemkin:skip-so2-normalization

Conversation

@DmitriyKorchemkin

Copy link
Copy Markdown
Contributor

This patch adds control over re-normalization for SO2/SE2 groups.

Rationale

For short sequences of operations (5..10 group products) the norm of the unit complex number representing the group element typically does not drift significantly.
In those cases, re-normalization can be omitted, avoiding extra cost of square roots and divisions for basic group operations.

For my problem (closed-form steering control computations) switching to unnormalized group operations yields ~3..4x increase in speed.

Implemented behavior

Normalization behavior is controlled by PreserveInvariant flag of the type.

Default is to keep normalization (current behavior), except SO2::inverse() and SO2::exp():

  • inverse() only changes the sign of the imaginary part, thus the norm is preserved automatically
  • exp() skips normalization under assumption that sin and cos values are consistent with each other

Conversions between (un-)normalized types are possible via operator() and SO2(SO2Base<OtherDerived> const&) constructors.

Group operation always returns unnormalized product if either of factors is unnormalized.

Implementation in the main branch performed normalization twice in operator*:

I have added a switch that checks if norm is exactly 1,0 after series-based approximation, but this might need to be relaxed.

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

inverse() cannot use conjugation alone once PreserveInvariant=false permits a non-unit complex value. For z with |z| != 1, the inverse is conj(z) / |z|^2; returning only conj(z) makes g * g.inverse() equal a scale of |z|^2 rather than identity. SE2Base::inverse() then also computes the wrong inverse translation from that value.

On exact head 628fa5aafce5151c5d38e54d253107d53a6f95d7, a small drift represented as (1.000001, 0) produces:

SO2 g*g.inverse():             (1.000002000001, 0)
SE2 g*g.inverse() rotation:    (1.000002000001, 0)
SE2 g*g.inverse() translation: approximately (-6e-6, -8e-6)

Using magnitude 2—the same deliberately unnormalized value used by the new tests—produces SO2 (4, 0) and SE2 translation (-9, -12) instead of identity.

Please compute the actual reciprocal for non-normalizing representations (and use it for the SE2 translation), then add SO2 and SE2 regressions asserting both g * g.inverse() and g.inverse() * g are identity for a drifted value. The submitted SO2/SE2 tests and the full locally available core suite pass (2/2 focused, 12/12 full), which confirms this path is currently untested.

AI disclosure: I used OpenAI Codex to help inspect the diff, construct and run the exact-head reproducer, and draft this review. I verified the mathematics, outputs, and conclusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants