Open olivierverdier opened 9 months ago
Currently,
GroupExponentialRetraction
depends on a typeActionDirectionAndSide
, but, unless I am mistaken, it should only depend onGroupActionSide
.Put differently, the
GroupExponentialRetraction{LeftBackwardAction}
andGroupExponentialRetraction{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 :+1: . 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 asinverse_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.
Currently,
GroupExponentialRetraction
depends on a typeActionDirectionAndSide
, but, unless I am mistaken, it should only depend onGroupActionSide
.Put differently, the
GroupExponentialRetraction{LeftBackwardAction}
andGroupExponentialRetraction{RightForwardAction}
make no sense.Indeed: in the code to
retract
, there is a call toinverse_translate_diff(G, p, p, X, conv)
, which is supposed to send the tangent vectorX
to the Lie algebra. But this only holds ifconv
is eitherLeftForwardAction
orRightBackwardAction
.Suggested solution: Implement the generally useful
translate_to_id
which translates a tangent vector to the identity (to the Lie algebra) like sothen change the implementation of
GroupExponentialRetraction
asthen essentially keep the current implementation of
retract
asNote 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 asinverse_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.