JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
368 stars 53 forks source link

`apply_diff_group` gives wrong sign? #669

Closed olivierverdier closed 10 months ago

olivierverdier commented 10 months ago

As far as I understand how apply_diff_group is supposed to work, when applied at the identity, it should sometimes reverse the sign, sometimes not (prior to v0.9.0, this was not the case, which might explain why the error cropped up).

using Manifolds

G = SpecialOrthogonal(3) # or just about any group

apply_at_id(G, ξ, d, s) = begin
    A = GroupOperationAction(G, (d,s))
    id = identity_element(base_group(G))
    return apply_diff_group(A, id, ξ, id)
end

_get_sign(::LeftAction, ::LeftSide) = 1 # former case
_get_sign(::LeftAction, ::RightSide) = -1 # new case
_get_sign(::RightAction, ::LeftSide) = -1 # new case
_get_sign(::RightAction, ::RightSide) = 1 # former case

test_apply_diff_group(G, ξ) = begin
    for d in [LeftAction(), RightAction()]
        for s in [LeftSide(), RightSide()]
            @show apply_at_id(G, ξ, d, s) ≈ _get_sign(d,s)*ξ
            end
        end
end

Now run test_apply_diff_group(G, rand(TangentSpace(G, identity_element(G)))). It should print true four times but doesn't.

I think apply_diff_group should be updated in light of the new implementation around group actions. It's a pretty serious bug, in my opinion. 😢

mateuszbaran commented 10 months ago

This indeed looks like a bug. I will look for the best solution and see if there could be other similar issues.

olivierverdier commented 10 months ago

I've tried to figure out the math that led to the confusion here. Introduce these notations for the translations (where +/- is for left/right action, and L/R is for left/right side) $$τ_p^{+L}(q) := pq$$ $$τ_p^{-R}(q) := qp $$ $$τ_p^{+R} := qp^{-1}$$ $$τ_p^{-L} := p^{-1}q$$

This induces the following relations between $τ_p^{\dots}(q)$ and $τ_q^{\dots}(p)$, namely: $$τ_p^{+L}(q) = τ_q^{-R}(p)$$ $$τ_p^{-R}(q) = τ_q^{+L}(p)$$ So far so good (corresponds to reverse_direction_and_side in the code), but now $$τ_p^{+R}(q) = τ_q^{+L}(p^{-1})$$ $$τ_p^{-L}(q) = τ_q^{-R}(p^{-1})$$ which explains why the current implementation of apply_diff_group cannot work. This calculation should also hint as to how to fix it, but I'm not sure exactly how.

mateuszbaran commented 10 months ago

Yes, those relations were used to derive the original code which wasn't carefully enough updated later.

mateuszbaran commented 10 months ago

After giving it some thought I think +R and -L actions should use inverse_translate_diff instead of translate_diff, and then it should work. I will try that in the evening.

olivierverdier commented 10 months ago

My guess is that it is going to be hard to avoid differentiating $p \mapsto p^{-1}$. For instance, if $f(p) := τ_p^{+R}(q)$, and $g =τ_q^{+L}$ (i.e., translate), then $f(p) =g(p^{-1})$, so $\langle Df, δp\rangle = -\langle Dg, p^{-1} δp p^{-1}\rangle$.

Maybe it's an idea to introduce a function inv_diff which differentiates the inverse?

The solution would then just be (for conv equal +R or -L):

apply_diff_group(G, a, X, p, conv) = translate_diff(G, p, a, inv_diff(G, a, X), switch_side(conv))
mateuszbaran commented 10 months ago

I see, inv_diff would really be helpful.

olivierverdier commented 10 months ago

I've made a bunch of tests for both inv_diff (for any group) and apply_diff_group (for any action, not only group action on itself). The tests pass on the few groups I've tested it against.

The important functions are

translate_diff_id(G, χ, ξ, conv) = translate_diff(G, χ, identity_element(G), ξ, conv)

"""
The left translation ``T_L(g,ξ) = gξ``.
"""
move(G, χ, ξ, ::LeftSide) = translate_diff_id(G, χ, ξ, (LeftAction(), LeftSide()))
"""
The right translation ``T_R(g,ξ) = ξg``.
"""
move(G, χ, ξ, ::RightSide) = translate_diff_id(G, χ, ξ, (RightAction(), RightSide()))

"""
Test the differential of the inverse on a Lie group.
Denote this inverse by ``I(g) := g^{-1}``.
If the left and right transports are ``T_L(g,ξ) := gξ``
and ``T_R(g,ξ) := ξg`` respectively, then
```math
⟨DI(g), T_L(g,ξ)⟩ = -T_R(g^{-1}, ξ)

and

⟨DI(g), T_R(g,ξ)⟩ = -T_L(g^{-1}, ξ)

""" test_invdiff(G, χ, ξ, conv) = begin χ = inv(G, χ) @test isapprox(TangentSpace(G, χ_), invdiff(G, χ, move(G, χ, ξ, conv)), -move(G, χ, ξ, switch_side(conv))) end

test_inv_diff_set(G, χ, ξ) = @testset "inv_diff" for side in [LeftSide(), RightSide()] test_inv_diff(G, χ, ξ, side) end

_get_side(::LeftAction) = RightSide() _get_side(::RightAction) = LeftSide()

_transporter(G, χ, ξ, dir) = move(G, χ, ξ, _get_side(dir))

""" This should hold for any group action A on any manifold. If you define π_x(g) := A(g, x) for g ∈ G and x ∈ M, and define, for ξ in Alg(G), T_R(g, ξ) := ξg (the right translation), and T_L(g, ξ) := gξ (the left translation), then we have the identity:

⟨Dπ_{x}(g), T(g, ξ)⟩ = ⟨Dπ_{A(g,x)}(1), ξ⟩

where, for a left action, T is the right translation, and for a right action, T is the left translation. """ test_apply_diff_group(A::AbstractGroupAction{TAD}, χ, ξ, p) where {TAD} = begin G = basegroup(A) p = apply(A, χ, p) v1 = apply_diff_group(A, χ, _transporter(G, χ, ξ, TAD()), p) v2 = apply_diff_group(A, identityelement(G), ξ, p) @test isapprox(TangentSpace(G, p_), v1, v2) end

test_apply_diff_groupset(G, χ, ξ, χ) = begin @testset "$dir, $side" for side in [LeftSide(), RightSide()], dir in [LeftAction(), RightAction()] test_apply_diffgroup(GroupOperationAction(G, (dir, side)), χ, ξ, χ) end end


Here is an example of how to use the tests on a particular group:
```julia
dim = 4
G = SpecialOrthogonal(dim)
# G = TranslationGroup(dim)

rng = Random.default_rng()

χ = rand(rng, G)
ξ = rand(rng, TangentSpace(G, identity_element(G)))

test_inv_diff_set(G, χ, ξ)

χ_ = rand(rng, G)
test_apply_diff_group_set(G, χ, ξ, χ_)

# Doesn't work, as `apply_diff_group` is not implemented for that action, but should work if it were implemented:
# V = Euclidean(dim)
# p = rand(V)
# A = RotationAction(V, G, LeftAction())
# test_apply_diff_group(A, χ, ξ, p)
mateuszbaran commented 10 months ago

Thank you, I will add it to our test set :heart: