Implement `translate_diff` and `inv_diff` for all groups (#679)

[olivierverdier]

The main work of implementing translate_diff for all groups is done.

The main method that has to be implemented on any given group is adjoint_action!, the rest is done automatically.

• [x] Replace translate_diff by adjoint_action
• [x] Implement inv_diff as well
• [x] Fixes for RealCircleGroup
• [x] Fixes for SpecialEuclidean
• [x] ~Add the extra erroring methods for SemiDirectProductGroup~ (no longer necessary)
• [x] Fix the "wrapped" SpecialEuclidean cases (for instance, ConnectionManifold(SpecialEuclidean(2), CartanSchoutenMinus()))
• [x] ~Implement inv_diff for Product/Power ~manifolds~ groups~ (no longer necessary)
• [x] ~Add tests with a product/power ~manifold~ groups containing a SpecialEuclidean or CircleGroup, two groups which do not use the left-invariant storage of tangent vectors~ (no longer necessary)
• [x] Implement adjoint_action for SpecialEuclidean
• [x] Use left invariant storage for SpecialEuclidean
• [x] Implement exp and log for Lie group based on exp_lie and log_lie

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.40%. Comparing base (676b0f5) to head (3dfde35). Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #683      +/-   ##
==========================================
- Coverage   99.58%   99.40%   -0.18%     
==========================================
  Files         114      114              
  Lines       11244    11320      +76     
==========================================
+ Hits        11197    11253      +56     
- Misses         47       67      +20     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[olivierverdier]

The PR is mostly implemented although it doesn't pass all the tests. Two problems essentially remain, which I'm not sure how I can fix:

1. The RealCircleGroup stores its elements as float, so the translate_diff! which is called after allocation doesn't work. Do you know how to fix it?
2. For a group such as SpecialEuclidean, everything works, but stops working when it is wrapped, for instance as a MetricSpace, because then the general translate_diff methods are called (instead of the SpecialEuclidean ones). Do you know how to fix that?

[kellertuer]

Hi, I just saw that the format check failed. If you install JuliaFormatter.jl (in your favourite environment) and you switch with your current folder into the Manifolds.jl base folder (we have an individual setup for formatter specified in that folder) you can run

using JuliaFormatter; format(".")

to format the code according to our rules. Actually the Formatter check does nothing else than that and checks whether the run changed the code (and if it changed on CI, it complains).

[kellertuer]

The other test that is currently not passed is the one that checked, that every PR changes the NEWS.md file to report what was changed with that PR (so that it is in the list of changes for the next released version).

[olivierverdier]

I can add the missing erroring method (task 5), but I leave the remaining tasks to you (explained in more details above), since I'm not really sure how to proceed.

[kellertuer]

That is one reason why I asked you to start working on this, only when working on this, one notices the remaining gaps in the ideas. I personally usually try to find some time nearly every day to work on these packages here, but for the next few weeks, also called semester end time, this will not be possible from my side, due to too many other duties.

[olivierverdier]

I’d be happy to finish this myself if you give me pointers as to how to proceed, that’s all I’m asking.

[mateuszbaran]

Thank you for this work, I will try to help finish this. I need to clarify a couple of things first.

If I understand correctly, adjoint_action for the left action is differential of $Ψ_p(q) = pqp^{-1}$, while for the right action it is the differential of $Ψ_p(q) = p^{-1}qp$? Also, why did you remove inv_diff and translate_diff for product and power manifolds? That would only work if inv_diff and translate_diff are never going to be specialized for particular manifolds.

1. The RealCircleGroup stores its elements as float, so the translate_diff! which is called after allocation doesn't work. Do you know how to fix it?

I will fix that myself.

[olivierverdier]

Thank you for this work, I will try to help finish this. I need to clarify a couple of things first.

Thank you, that's much appreciated!

If I understand correctly, adjoint_action for the left action is differential of Ψp(q)=pqp−1, while for the right action it is the differential of Ψp(q)=p−1qp?

Yes, exactly. Let's fix $ψ_p(q) := p q p^{-1}$. Then the adjoint action is the derivative of $ψ_p$ at the identity. The right action is the left adjoint action, but at $p^{-1}$ so the derivative of $ψ_{p^{-1}}$ at the identity, as you say (is is the same relation as between any other left/right actions).

Also, why did you remove inv_diff and translate_diff for product and power manifolds? That would only work if inv_diff and translate_diff are never going to be specialized for particular manifolds.

Excellent point, I made a mistake there. It would only work if all the group use the left-invariant storage, which shouldn't be assume indeed.

1. The RealCircleGroup stores its elements as float, so the translate_diff! which is called after allocation doesn't work. Do you know how to fix it?

I will fix that myself.

Thanks a lot!

[olivierverdier]

Also, why did you remove inv_diff and translate_diff for product and power manifolds? That would only work if inv_diff and translate_diff are never going to be specialized for particular manifolds.

I've tried to rectify that in 6f57baf.

However, the fact that the tests didn't catch that indicates that all the tests for product/power manifolds never involve groups with no left-invariant storage, so it could be an idea to add such tests?

[olivierverdier]

Unless I am mistaken, inv_diff was not implemented for product/power ~manifolds~ groups, but it should be (similarly to translate_diff) if one of the components does not use the left-invariant storage. I add that as a task.

[kellertuer]

just to aim for the same precision as you do – software wise we try to split for some time, to have first manifolds (e.g. power manifold and product manifold) and have the Lie part as an add-on (not yet 100% consistent, since some time all new stuff is, cf. rotations and their split from SO(n)).

So with that in mind, please implement inv_diff for the product and power group and not the manifold :)

[olivierverdier]

So with that in mind, please implement inv_diff for the product and power group and not the manifold :)

Yes, I meant "product/power group", as inv_diff doesn't make sense for manifolds.

For the record, before this PR, inv_diff was not available at all for product/power groups, as far as I know.

[kellertuer]

Sure, sometimes we (a) might not have implemented existing methods for a new manifold directly or (b) when implementing a new method we might not have taken the whole ecosystem under revision; both cases might espcially happen, when a method is still new and needs more experience or an “external contributor“ started them.

[mateuszbaran]

Thanks for clarification, I should have some time tomorrow to work on this PR.

[mateuszbaran]

I've fixed CircleGroup and RealCircleGroup. They can be sometimes a little annoying to work with because numbers can't be allocated and we need specialized non-allocating methods.

[kellertuer]

HI @olivierverdier, do you have plans and time to continue this? That would be great. We could help merging the current master here for example.

I think in the long run it would be really great to move the Lie group parts to a dedicated LieGroups.jl package (depedingin on ManifoldsBase and Manifolds – we could also do a LieGroupsBase.jl just depending on ManifoldsBase, but that can also be done in a later step). Such a “restart” is maybe also a good point to rethink a few of the design choices.

[olivierverdier]

No, I don't have plans to work on this. The main issue is that some of the groups store tangent vectors differently than others (I think it is SpecialEuclidean and RealCircleGroup), and that creates a lot of unnecessary complications.

My personal solution: I'm running my own version of Manifolds.jl which is this PR along with these complications removed and I'm happy with that solution so far.

[kellertuer]

That is interesting to know.

I think the reason here might be that until now we often inherited the representation from the manifold – which for some manifolds might not make sense, you are right.

I think switching to your variant might also be a good idea, with the small caveat, that this would be breaking. on the other hand, if your new representation is worth that, maybe it is worth releasing a breaking change.

[kellertuer]

If you do not plan to propose a breaking change here, we should maybe see how we can adapt this PR (maybe @mateuszbaran you can take a look)? And we should close this PR.

[olivierverdier]

The essential difference in my version is that I just removed the translate_diff! method in semidirect_product_group.jl. This allows me to use the SemiDirectProductGroup the way I wanted, with left translated vectors as implemented in this PR.

(I also had to remove the get_vector, get_vector! and get_coordinates! method for the VeeOrthogonalBasis, in that same file, for some reason, I can't remember why)

[mateuszbaran]

One possible approach would be to allow both representations and have a flag in Lie groups that tells which one is used. That's something I could do but there are a few other things I'd like to do first (some extensions and improvements to Manopt, maybe pushing a bit forward my work on fiber bundles and state estimation).

[kellertuer]

If we allow both, we could do that maybe with different tangent vector types? Until now that was the way we distinguished different tangent vector types.

[mateuszbaran]

That's possible but it would be quite inconvenient to wrap every tangent vector when someone only works in the left-invariant setting.

[kellertuer]

That one could counter a bit by making our current modelling of “what does an array represent” a bit more public (I am no sure how public that is now directly), so a user can easily (or easier) specify his/her default behaviour of what tangent vectors are usually represented in.

[mateuszbaran]

That one could counter a bit by making our current modelling of “what does an array represent” a bit more public (I am no sure how public that is now directly),

Currently it's up to each manifold what an array means but that is a part of the public interface (of that particular manifold).

so a user can easily (or easier) specify his/her default behaviour of what tangent vectors are usually represented in.

And that would have to be done through a flag in the manifold/Lie group as far as I can see.

[kellertuer]

I see. What I meant was is that currently we have lines like

ManifoldsBase.@default_manifold_fallbacks Hyperbolic HyperboloidPoint HyperboloidTVector value value

and sure these are macros that define the default (fallback) that specify what arrays are the same as. We could also have a function dispatch for that instead of a parameter maybe?

Sure in the end we have to store the information somewhere, just that maybe a dispatch is nicer than a parameter in a struct (since we would have to add that to a lot of structs).

But this is maybe also a discussion beyond this PR. The question here is, how do we resolve the bugs mentioned / noticed here? Or is that just a matter of how to represent tangent vectors?

[mateuszbaran]

and sure these are macros that define the default (fallback) that specify what arrays are the same as. We could also have a function dispatch for that instead of a parameter maybe?

Sure in the end we have to store the information somewhere, just that maybe a dispatch is nicer than a parameter in a struct (since we would have to add that to a lot of structs).

That dispatch is very different though, I don't see how we could re-use a trick like that for having two different representations of tangent vectors.

But this is maybe also a discussion beyond this PR. The question here is, how do we resolve the bugs mentioned / noticed here? Or is that just a matter of how to represent tangent vectors?

IIRC there were no bugs addressed here, only change of representation and implementation of a few functions that are currently not implemented in Manifolds.jl for some groups. New implementations can be added without change of representation but only in a less nice way.

[kellertuer]

You are right, it might not be super easy.

Ah, I did not see that in the changes so clearly. But then we can close this PR?

[mateuszbaran]

Closing this PR would make it easier to forget about it, and I don't want to forget. It has valuable contributions to Lie groups which I would like to incorporate into Manifolds.jl next time I have time to work on this area.