Wrong parameter for GroupExponentialRetraction

[olivierverdier]

Currently, GroupExponentialRetraction depends on a type ActionDirectionAndSide, but, unless I am mistaken, it should only depend on GroupActionSide.

Put differently, the GroupExponentialRetraction{LeftBackwardAction} and GroupExponentialRetraction{RightForwardAction} make no sense.

Indeed: in the code to retract, there is a call to inverse_translate_diff(G, p, p, X, conv), which is supposed to send the tangent vector X to the Lie algebra. But this only holds if conv is either LeftForwardAction or RightBackwardAction.

---

Suggested solution:
Implement the generally useful translate_to_id which translates a tangent vector to the identity (to the Lie algebra) like so

_conv_from_side(::LeftSide) = LeftForwardAction()
_conv_from_side(::RightSide) = RightBackwardAction()

translate_to_id(G, p, X, side::GroupActionSide) = translate_diff(G, p, p, X, switch_side(_inv_conv_from_side(side)))

then change the implementation of GroupExponentialRetraction as

struct GroupExponentialRetraction{D<:GroupActionSide} <: AbstractRetractionMethod end

function GroupExponentialRetraction(side::GroupActionSide=LeftSide())
    return GroupExponentialRetraction{typeof(side)}()
end

group_action_side(::GroupExponentialRetraction{D}) where {D} = D()

then essentially keep the current implementation of retract as

function retract(
    ::TraitList{<:IsGroupManifold},
    G::AbstractDecoratorManifold,
    p,
    X,
    method::GroupExponentialRetraction,
)
    side = group_side(method)
    Xₑ = translate_to_id(G, p, X, side)
    pinvq = exp_lie(G, Xₑ)
    q = translate(G, p, pinvq, _conv_from_side(side))
    return q
end

---

Note 1 there is probably exactly the same problem (solved in the same manner) for GroupLogarithmicInverseRetraction.

Note 2 inverse_translate_diff may be superfluous in general, or at least it should be implemented as inverse_translate_diff(G, p, q, X, conv) = inverse_translate_diff(G, p, q, X, switch_direction(conv)).

Let me know if the problem and/or the solution make any sense to you.

[mateuszbaran]

Currently, GroupExponentialRetraction depends on a type ActionDirectionAndSide, but, unless I am mistaken, it should only depend on GroupActionSide.

Put differently, the GroupExponentialRetraction{LeftBackwardAction} and GroupExponentialRetraction{RightForwardAction} make no sense.

You're right, GroupExponentialRetraction and GroupLogarithmicInverseRetraction should be restricted to left and right side.

Suggested solution:
Implement the generally useful translate_to_id which translates a tangent vector to the identity (to the Lie algebra) like so

This looks like the right solution 👍 . It's technically breaking but I'd consider it a bugfix so we could do it without a breaking release.

Note 2 inverse_translate_diff may be superfluous in general, or at least it should be implemented as inverse_translate_diff(G, p, q, X, conv) = inverse_translate_diff(G, p, q, X, switch_direction(conv)).

Yes, it is currently superfluous. Originally when it was introduced we didn't have right-forward and left-backward actions so it was useful but right now is should be deprecated in favor of translate_diff with these conventions.

[kellertuer]

I did not fully follow up on the 0.10 rework, but was this one resolved?

[olivierverdier]

No, this is independent from the 0.10 rework. This is a bug that still is to be fixed: using GroupExponentialRetraction with the wrong arguments will give nonsensical results.

[kellertuer]

Ah I see, then let's keep this open until we remove the group exponential retraction here, when we have an analogue in Lie groups.
this might then still be an effect of the introduction of the side part, which I have still not yet understood and was maybe introduced a bit too uncareful here. But let's keep this open then to have that further in mind.

[olivierverdier]

Actually, i realise now that these retractions should not depend on any such parameter at all. I will try to fix that.