Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply_diff_group gives wrong sign? #669

Closed
olivierverdier opened this issue Oct 26, 2023 · 8 comments · Fixed by #673
Closed

apply_diff_group gives wrong sign? #669

olivierverdier opened this issue Oct 26, 2023 · 8 comments · Fixed by #673
Labels
bug Something isn't working

Comments

@olivierverdier
Copy link
Contributor

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
Copy link
Member

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

@mateuszbaran mateuszbaran added the bug Something isn't working label Oct 26, 2023
@olivierverdier
Copy link
Contributor Author

olivierverdier commented Oct 27, 2023

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
Copy link
Member

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

@mateuszbaran
Copy link
Member

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
Copy link
Contributor Author

olivierverdier commented Oct 27, 2023

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
Copy link
Member

I see, inv_diff would really be helpful.

mateuszbaran added a commit that referenced this issue Oct 27, 2023
@mateuszbaran mateuszbaran linked a pull request Nov 3, 2023 that will close this issue
2 tasks
@olivierverdier
Copy link
Contributor Author

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

  • test_inv_diff_set(G, g, ξ), where $g$ is a group element and $ξ$ in the Lie algebra
  • test_apply_diff_group(A, g, ξ, p), where $A$ is any action, and $p$ is a point on the manifold
  • test_apply_diff_group_set(G, g1, ξ, g2), which runs test_apply_diff_group on the four actions on a group on itself
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
``` math
⟨DI(g), T_R(g,ξ)⟩ = -T_L(g^{-1}, ξ)
```
"""
test_inv_diff(G, χ, ξ, conv) = begin
    χ_ = inv(G, χ)
    @test isapprox(TangentSpace(G, χ_), inv_diff(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:
```math
⟨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 = base_group(A)
    p_ = apply(A, χ, p)
    v1 = apply_diff_group(A, χ, _transporter(G, χ, ξ, TAD()), p)
    v2 = apply_diff_group(A, identity_element(G), ξ, p_)
    @test isapprox(TangentSpace(G, p_), v1, v2)
end


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

Here is an example of how to use the tests on a particular group:

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
Copy link
Member

Thank you, I will add it to our test set ❤️

mateuszbaran added a commit that referenced this issue Nov 6, 2023
* Inverse of differential, fix for apply_diff_group issue

* news entry

* `inverse_translate_diff` fix

* Fixes

* Another fix

* test for issue #669

* improve coverage

* more tests

* some tests and figuring out

* fix inv_diff

* fix translate_diff on general unitary

* remove some tests that don't apply to all actions

* why fail there?

* tests

* add some tests

* this was right

* Giles is helpful

* fix circle group inv_diff

* add some more tests

* add pullbacks for group inverse

* test for translations

* update news

* fix tests

* fix the other test

* coverage false positives

* update version
mateuszbaran referenced this issue Nov 7, 2023
* BoundaryValueDiffEq v5 in docs

* this can still be a part of 0.9.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants