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.

• Replace translate_diff by adjoint_action
• Implement inv_diff as well
• Fixes for RealCircleGroup
• Fixes for SpecialEuclidean
• Add the extra erroring methods for SemiDirectProductGroup (no longer necessary)
• Fix the "wrapped" SpecialEuclidean cases (for instance, ConnectionManifold(SpecialEuclidean(2), CartanSchoutenMinus()))
• Implement inv_diff for Product/Power manifolds groups (no longer necessary)
• 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)
• Implement adjoint_action for SpecialEuclidean
• Use left invariant storage for SpecialEuclidean
• Implement exp and log for Lie group based on exp_lie and log_lie

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 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.

[kellertuer]

Thanks for your thorough contribution – much appreciated.

I looked through the code, and I added a few small questions.

besides that it would maybe be great to document the left-right-action thing on group.md a bit more so an only midly-Lie-group-experienced person like me has a chance to read up on the detail fixed here?
If that would be possible that'd be great.

[olivierverdier]

besides that it would maybe be great to document the left-right-action thing on group.md a bit more so an only midly-Lie-group-experienced person like me has a chance to read up on the detail fixed here?

I agree. I'll try to do that.

[olivierverdier]

besides that it would maybe be great to document the left-right-action thing on group.md a bit more so an only midly-Lie-group-experienced person like me has a chance to read up on the detail fixed here?

I agree. I'll try to do that.

Done. Let me know if it is understandable.

[mateuszbaran]

I'm going to merge this PR into #732 next week and then make necessary adaptations there. Thank you again for this contribution 👍

[mateuszbaran]

By the way, I've already merged this PR into #732 and I will polish it there.

[mateuszbaran]

In my PR I've restricted these log and exp methods to semidirect product groups with with left-invariant tangent vector representation. On many groups we have optimized implementation on the manifold so I'd prefer to have that to be picked up first before this relatively generic fallback.

[olivierverdier]

The solution here is to call the exponential/logarithms defined in groups/connections.jl. It's the only acceptable exp and log on a group manifold. This way, it will work for all groups, regardless of the tangent vector representation. Should I fix this in this PR?

[mateuszbaran]

I know your solution is correct, this is mostly about performance (and a bit about consistency, see the next paragraph). Substantial amount of work has been put to make practically relevant calls fast but unfortunately it's not gathered in an easy to run benchmark suite, so verifying what could have been slowed down and fixing issues would take me hours. So, unless you know of a case where my solution gives a wrong result for an existing group, I'd prefer to keep it like that for now.

Additionally, considering your implementation here makes me rethink our entire group -> manifold pass-through and it's not really something I'd like to do in the near future. If we don't pass exp and log to the manifold, why do we pass vector transports, inner product, retractions, inverse retractions, Riemann tensor, affine connection and so on? Maybe the current solution is not ideal but reworking it would take a very long time and I just don't see enough benefits.

Should I fix this in this PR?

Since I've already merged your branch, it would be easier for me if you made any further fixes on top of the branch from #732 .

[kellertuer]

vector transport and retractions (and their inverses) are still valid, at least after turning back to tangent vectors in tangent spaces (not 100% sure about Lie algebra things); but it might be beneficial to maybe not pass through exp/log. But I agree that that is something that has to be considered carefully and in the near future I do not have the capacity for that.

[olivierverdier]

Thank you for your feedback and explanations! I hope I did not completely misunderstand.

1. Before my PR, the exp that was used was not necessary the group exponential (the one in groups/connections.jl). For instance, for semidirect products, the exponential of the underlying product manifold was used, which was wrong, because it was not invariant. My implementation fixed that, but was also flawed since it assumed left invariant representation. This is easily fixed as I mentioned above. I'd be happy to do that.

2. I'm not sure I understand the implications of what you call the "pass through" here... I'd be happy to be enlightened.
   As far as speed is concerned, exp is always an expensive operation, and I doubt that the overhead of function calls makes any difference. Am I wrong to assume that?

3. The only optimized exp that I can find in the code is that of GeneralLinear. It comes from a choice of left invariant metric, and I'm pretty sure it is not the same as the invariant exponential in groups/connections.jl. (So that optimized exponential is not invariant, unless I am mistaken) Have I missed any other optimized exp implementation?

[kellertuer]

I think exp_inv and log_inv would also be good ideas – they would be a bit more specific than wrapping the manifold in a metric (as I proposed in my last comment) and dispatch with exp on that. I would be fine with both. Maybe the wrapping idea avoids introducing (yet) another function name.

[olivierverdier]

I get it now (I think): you want exp to be defined from a metric connection.

If exp is defined from a nonmetric connection, you are quite sure that something is going to break (because Lie groups are built on top of metric manifolds).

We could add a keyword argument to LIeGroup that says: I want to have the Olivier-approach (name to be found still) Then the manifold that is passed would be wrapped in a MetricManifold with the corresponding metric (the one that is the best one in Oliviers sense) and on that dispatch the default from Olivier would work.

Yes, that would be nice, but it may not be possible, since the invariant connection is not necessarily metric (example: GL, semidirect products).

It isn't entirely accurate; the connection is currently arbitrary but I'm open to changing that, just in a consistent manner.

For a metric connection, there is no consistent solutions, your solution is as good as any other. If you agree with (possibly) nonmetric connections, the invariant one is the only consistent choice across all Lie groups.

You made an incorrect assumption about how Manifolds.jl operates and refuse to adjust your expectations despite my detailed explanations.

I guess I did. Sorry for that.

Whenever I was talking about changing the logic, I meant exp not corresponding to the connection chosen for a group. This is the fundamental logic of exp. If you don't like it, don't use exp. This is one part where I see no point in changing anything about Manifolds.jl: exp must come from a consistent choice of connection. We can change which connection it is but invariance is not a requirement for exp.

You mean a metric connection. Tu sum it up, you prefer a metric noninvariant connection over an invariant nonmetric connection.

This is our disagreement in a nutshell.

Maybe a good way to resolve this disagreement would be to introduce exp_inv and log_inv (invariant exp and invariant log) which would be implemented in the exact way you described?

Yes, that would be very nice. I think those two functions deserve to be defined somewhere.

---

1. Do you want to convince us that we are stupid? I feel your tone if often quite agressive

It's just a misunderstanding, a disagreement. I like to understand things. I didn't and it's my fault, I'm slow, it makes me sound aggressive. But I think I get it now. Nobody is stupid here, or maybe we're all stupid in the face of maths.

I would really recommend to start Liegroups.jl and “do Lie Groups right” following your arguments and use Manifolds.jl as a kind of backend (for example for point checks or such).
I am even on board helping with that a bit, setting everything up here in JuliaManifolds, CI / docs / ... / review PRs.

I really appreciate that. I kind of already started actually. I'll let you know how it goes.

Tell me if we finally understand each other (i.e.: you only want exp defined from a metric connections, otherwise you know something will break). If we do, we can resolve this conversation.

[kellertuer]

I am fine with us all being stupid ;)
Sorry your tone (and repetition as it came across to me) was getting to me as agressive – thanks for clarifying that this was not meant like that.

My main problem until now was, that to me this thread slowly read like we are repeating arguments again and again.
But if you are fine with the exp_inv/lie_inv approach, that is fine with me as well. And yes, I think we by now understand each other, what we want from exp and why that is different fro yours.

For the LieGroups.jl idea; I think a nice thing would also be that we do not “overcrowd” Manifolds.jl but modularise features a bit. If you want and have done some work on that, as I said, I am very happy to help with all technical details in setting that up; I would prefer that in JuliaManifolds/ – just to not have a package in a persons namespace – but you would be admin of that package as well of course.

[mateuszbaran]

I get it now (I think): you want exp to be defined from a metric connection.

Yes, we chose the connections for GL and SL because we prefer the defaults to be metric non-invariant connections over non-metric invariant connections. Not all exp methods come from a metric connection but we generally prefer metric connections because they give more rich structure required by certain optimization or statistical algorithms; moreover, not all manifolds are Lie groups.

You mean a metric connection. Tu sum it up, you prefer a metric noninvariant connection over an invariant nonmetric connection.

This is our disagreement in a nutshell.

Yes, that's correct.

Yes, that would be very nice. I think those two functions deserve to be defined somewhere.

OK, great, then I will add them in my PR and mention in the new tutorial.

1. Do you want to convince us that we are stupid? I feel your tone if often quite agressive

It's just a misunderstanding, a disagreement. I like to understand things. I didn't and it's my fault, I'm slow, it makes me sound aggressive. But I think I get it now. Nobody is stupid here, or maybe we're all stupid in the face of maths.

For me developing this library is a great learning opportunity. I'm not a mathematician, and primarily see various branches of differential geometry and Lie groups as a nice language for generalizing classical numerical algorithms. I'm sometimes wrong, but when someone shows me why I'm wrong I can learn something new. There are definitely dozens of people who would do much better job developing JuliaManifolds but they are too busy with more important stuff.

I'm happy when people find JuliaManifolds useful and I generally consider any suggestions regarding interface and functionality of libraries. However, it's impossible to satisfy everyone and we have to set some priorities.

I generally don't mind the tone of messages I get about JuliaManifolds, either yours and from other people. I know it's sometimes a real hassle to express in a positive way, even more so if someone is not a native speaker.

Tell me if we finally understand each other (i.e.: you only want exp defined from a metric connections, otherwise you know something will break). If we do, we can resolve this conversation.

Yes, we prefer metric connections if they are available; I think we've managed to resolve the misunderstanding here finally 🙂 .

For the LieGroups.jl idea; I think a nice thing would also be that we do not “overcrowd” Manifolds.jl but modularise features a bit. If you want and have done some work on that, as I said, I am very happy to help with all technical details in setting that up; I would prefer that in JuliaManifolds/ – just to not have a package in a persons namespace – but you would be admin of that package as well of course.

Not-so-fun fact: https://github.com/yuehhua/LieGroups.jl already exists, is registered and looks abandoned. I think it would be fine to develop Lie groups in a separate package but currently we have a bunch of functionality here and maybe some part of it should be moved there?

[kellertuer]

Oh, I did not see that package, well, if ours gets a bit more serious we could reach out to hopefully take over the name, if that is abandoned. It also seems to not cover many (only one?) Lie group.

[olivierverdier]

I realised I forgot to redefine inverse_translate_diff. The default (for left invariant representation) should be:

inverse_translate_diff!(G, Y, ::Any, ::Any, X, ::LeftForwardAction,) = copyto!(G, Y, X)
inverse_translate_diff!(G, Y, ::Any, ::Any, X, ::RightForwardAction,) = copyto!(G, Y, X)
inverse_translate_diff!(G, Y, p, ::Any, X, ::LeftBackwardAction,) = adjoint_action!(G, Y, p, X, RIghtAction())
inverse_translate_diff!(G, Y, p, ::Any, X, ::RightBackwardAction,) = adjoint_action!(G, Y, p, X, LeftAction())

The current definition is not wrong, and it works for all representations, but it is very inefficient, as it potentially inverts a matrix three times instead of once, or once instead of zero.

This change does not have to occur within the scope of this PR, it can be done later, maybe in a separate issue.

[kellertuer]

We coudls surely still ad it to the 0.10 PR that is still open?

[mateuszbaran]

Thanks, I will add that to the 0.10 PR.

[mateuszbaran]

Uh, it seems to cause lots of ambiguities, so maybe let's resolve that in a new PR since it not breaking. Anyway, here is the adapted code:

function inverse_translate_diff!(
    ::TraitList{<:IsGroupManifold{<:AbstractGroupOperation,LeftInvariantRepresentation}},
    G::AbstractDecoratorManifold,
    Y,
    ::Any,
    ::Any,
    X,
    ::LeftForwardAction,
)
    return copyto!(G, Y, X)
end
function inverse_translate_diff!(
    ::TraitList{<:IsGroupManifold{<:AbstractGroupOperation,LeftInvariantRepresentation}},
    G::AbstractDecoratorManifold,
    Y,
    ::Any,
    ::Any,
    X,
    ::RightForwardAction,
)
    return copyto!(G, Y, X)
end
function inverse_translate_diff!(
    ::TraitList{<:IsGroupManifold{<:AbstractGroupOperation,LeftInvariantRepresentation}},
    G::AbstractDecoratorManifold,
    Y,
    p,
    ::Any,
    X,
    ::LeftBackwardAction,
)
    return adjoint_action!(G, Y, p, X, RightAction())
end
function inverse_translate_diff!(
    ::TraitList{<:IsGroupManifold{<:AbstractGroupOperation,LeftInvariantRepresentation}},
    G::AbstractDecoratorManifold,
    Y,
    p,
    ::Any,
    X,
    ::RightBackwardAction,
)
    return adjoint_action!(G, Y, p, X, LeftAction())
end

[mateuszbaran]

I've merged #732 so this PR is already merged indirectly into master.