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

`inv_diff` possibly missing for `SpecialEuclidean`? #676

Closed olivierverdier closed 10 months ago

olivierverdier commented 10 months ago

Neither the tests I provided, nor the following passes for SpecialEuclidean:

test_inv_diff(rng, G) = begin
    id = identity_element(G)
    alg = TangentSpace(G, identity_element(G))
    ξ = rand(rng, alg)
    ξ_ = apply_diff_group(GroupOperationAction(G, (LeftAction(), RightSide())), id, ξ, id)
    @test isapprox(alg, ξ, -ξ_)
end

and

test_inv_diff(Random.default_rng(), SpecialEuclidean(3))

produces a MethodError: inv_diff!... is ambiguous, which, I suppose, means inv_diff! is not implemented for the SpecialEuclidean group?

mateuszbaran commented 10 months ago

Yes, inv_diff! is just not implemented yet for SpecialEuclidean. MethodErrors usually mean that something isn't implemented. It wouldn't be too hard to add, you could use the embedding of SE(n) in GL(n+1) and the existing implementation of inv_diff! for matrix groups.

olivierverdier commented 10 months ago

Good point, I'll do that.

olivierverdier commented 10 months ago

Actually, I don't understand the implementation of both inv_diff and inv_diff! in multiplication_operation.jl. To me (and according to the docstring of inv_diff in this file), the derivative of the inverse is $-p^{-1}Xp^{-1}$, but what is computed instead is $-p X p^{-1}$. Or did I misunderstand something?

mateuszbaran commented 10 months ago

The derivative of matrix inverse is indeed $Y \rightarrow -p^{-1}Yp^{-1}$, but we represent tangent vectors in the Lie algebra. So $Y=pX$, and we need to transport the resulting vector back to identity, like $Z \rightarrow (p^{-1})^{-1}Z$ (see project and embed for matrix Lie groups). So in total we have $X \rightarrow (p^{-1})^{-1} (-p^{-1}) pX p^{-1} = -pXp^{-1}$.

This representation of tangent vectors confuses me every time I have to derive some new implementation and I'm not sure how to explain it more clearly.

olivierverdier commented 10 months ago

Ah yes, that makes sense, thank you. You explain very well, but ~I guess it could be part of the docstring, as it is an important piece of information for the user?~

Also: then it is in fact minus the adjoint action of p on X. What about reusing that implementation? Would that make sense? Something like

inv_diff = -adjoint_action
olivierverdier commented 10 months ago

Ooops, I'm sorry, this is very clearly stated in the docstring, I just read it too quickly.

olivierverdier commented 10 months ago

If this representation of tangent vectors on Lie groups holds throughout, you could just define inv_diff = -adjoint_action for all groups at once?

mateuszbaran commented 10 months ago

Hm, I didn't notice that. It makes some sense though. I think we can have -adjoint_action as a default implementation of inv_diff. It would likely be much slower, though, than a specialized implementation for SE(n). And that generic default likely wouldn't see much use if we had the specialized version for SE(n).

olivierverdier commented 10 months ago

For SE(n), you can just use the already efficient implementation of adjoint_action, right? Why not?

mateuszbaran commented 10 months ago

Currently we only have an efficient implementation of adjoint_action on SE(3) but sure, adding efficient adjoint action for other n and using that in SE(n) should be fine as well. adjoint_action is a bit more general though so I'm not sure how much work it would be to work it out for n > 3?

mateuszbaran commented 10 months ago

Anyway, I think it's a good enough point to implement the fallback. I can do that soon.

olivierverdier commented 10 months ago

Hang on, if the representation by the left holds all the time for all the groups, then we simply have the identity inv_diff = -adjoint_action. If you implemented inv_diff but not adjoint_action somewhere, then switch the sign, and call it adjoint_action instead, as that's what it is, right?

mateuszbaran commented 10 months ago

I don't really want to force the assumption about left-translation representation of tangent vectors too much. I think it should be possible to implement a group for which it doesn't hold, though likely some default implementations would have to be overridden.

If you implemented inv_diff but not adjoint_action somewhere, then switch the sign, and call it adjoint_action instead, as that's what it is, right?

I was going to do the fallback the other way: implement inv_diff using adjoint_action, mostly because adjoint_action was introduced earlier.

mateuszbaran commented 10 months ago

And IIRC all groups in Manifolds.jl use the left-invariant representation in Lie algebra.

olivierverdier commented 10 months ago

I was going to do the fallback the other way: implement inv_diff using adjoint_action, mostly because adjoint_action was introduced earlier.

Yes, it has to be this way: the actual computation is that of adjoint_action. That computation is always right, independently of any representation.

Now, if the tangent vectors are represented by left-invariant representation, then inv_diff = -adjoint_action, but that depends on that crucial fact.

olivierverdier commented 10 months ago

And IIRC all groups in Manifolds.jl use the left-invariant representation in Lie algebra.

Then there is something strange with the implementation of translate_diff for semi-direct products, then, since translate_diff is trivial for forward actions (see for example the implementation for GeneralUnitaryGroup). Isn't there something off?

mateuszbaran commented 10 months ago

I just checked that and it looks like this is caused by tangent vectors in semidirect product groups not being stored in the left-invariant fashion, which I forgot about -- sorry.