Inconsistency with `switch_direction`?

[olivierverdier]

Let me first clarify a point about left and right actions. From a left action on a manifold, (g,x) ↦ L(g,x), one can always construct a right action, namely: L'(g,x) := L(g⁻¹,x). (A right action can also be transformed into a left action in this manner.)

Now, on a group, there are two distinct actions, stemming from multiplying on the left, or on the right. Put differently, one can say that there are two distinct left actions, L₁, and L₂ defined by L₁(g,g') := g g' and L₂(g,g') := g' g⁻¹. Now, according to the process above, both each give rise to two distinct right actions, namely L₁', and L₂'.

Here comes the inconsistency.

For the RotationAction, the result of switch_direction on the action is to map L to L' (where L is the rotation action).

But for the GroupOperationAction, the result of switch_direction is to map L₁ to L₂', instead of mapping L₁ to L₁'.

It would perhaps be nice if it was better documented, or, better, fixed to make it consistent?

[sethaxen]

Could you provide a MWE demonstrating the inconsistent behavior?

[olivierverdier]

Sure:

check_action(A::AbstractGroupAction, a, x) = apply(switch_direction(A), a, x) ≈ apply(A, inv(a), x)
rand_check_action(A::AbstractGroupAction) = check_action(A, rand(base_group(A)), rand(group_manifold(A)))

AR = RotationAction(Euclidean(4), SpecialOrthogonal(4))
AG = GroupOperationAction(G)

rand_check_action(AR) # true
rand_check_action(AG) # false

[olivierverdier]

It's a broader problem than switch_direction: it is the fact that the ActionDirection has a different meaning for GroupOperationAction and for other actions, in particular for RotationAction.

In fact, for GroupOperationAction, one would need two booleans to differentiate between the four actions I described above, something like Right/Left (as for any action), but also maybe Forward/Backward, or some such. And switch_direction would still be inconsistent (because, as per the current implementation, it changes action and direction, for instance, (Forward, Left) is mapped to (Backward, Right)), so the name shoud perhaps also be changed.

[mateuszbaran]

Good point, I haven't thought about distinguishing all four actions for GroupOperationAction but it would make sense. I generally don't use switch_direction and just construct the action I need. So, L₁ would be left forward, L₁' would be right forward, L₂ would be left backward and L₂' would be right backward? I think switch_direction could then get another argument that would say which thing should be switched (left/right, forward/backward or both).

[mateuszbaran]

FYI, I will make a PR addressing this issue later this week.

[olivierverdier]

Brilliant, thank you!

[olivierverdier]

You would also have to change GroupOperationAction to create the forward/backward version as well...

But, another more interesting possibility is to make switch_direction completely consistent (always changing left/right for the same action). One can then create the four actions as

• GroupOperationAction(G, LeftAction()) -> L₁
• GroupOperationAction(G, RightAction()) -> L₂'
• switch_direction(GroupOperationAction(G, LeftAction())) -> L₁'
• switch_direction(GroupOperationAction(G, RightAction())) -> L₂

It is even possible to maintain backward compatibility: introduce, say, reverse_direction which systematically changes the direction of an action in general. (switch_direction can then be deprecated, or maybe used only for group actions, to switch between L₁<->L₂)

[mateuszbaran]

I would like to discuss the design a bit before committing to it. Currently the most reasonable option seems to be to define four actions:

abstract type ActionDirection end

struct LeftForwardAction <: ActionDirection end
struct RightForwardAction <: ActionDirection end
struct LeftBackwardAction <: ActionDirection end
struct RightBackwardAction <: ActionDirection end

and then for compatibility

const LeftAction = Union{LeftForwardAction,LeftBackwardAction}
LeftAction() = LeftForwardAction()
const RightAction = Union{RightForwardAction,RightBackwardAction}
RightAction() = RightBackwardAction()

Does it sound good?

cc @sethaxen @kellertuer

[kellertuer]

To me that looks good.

[olivierverdier]

I'm sorry, I have to react to this change. This change makes little sense to me (sorry).

1. There should ever be just two action directions: left and right. (They are in one to one correspondence, so, mathematically, one can just as well focus on left actions only)
2. On a group, there happens to be two left actions (which are now named Forward and Backward, this is fine)

But now there are there four action directions? What would that even mean? For any action other than on a group from itself, the distinction Forward/Backward simply makes no sense at all, does it?

Consider this code:

V = Euclidean(3)
G = SpecialOrthogonal(3)
A = RotationAction(V,G, RightAction())

Now A is represented as RotationAction(Euclidean(3; field = ℝ), SpecialOrthogonal(3), RightBackwardAction()). What does Backward even mean here? Shouldn't it just be a RightAction?

[kellertuer]

Now you lost me. The change does exactly what you proposed on June 29, so then your post from June 29 does not make sense?

And by @mateuszbaran s post RightAction() = RightBackwardAction() those two are equivalent by definition. So that is just a RightAction, because by definition they are equal, even in memory (that is ===).

[mateuszbaran]

Hm, it was just easier to keep using ActionDirection for forward and backward group operation actions, and I assumed this could also be useful for some other actions (though obviously not all of them). What would you propose?

What does Backward even mean here? Shouldn't it just be a RightAction?

Good point, this is indeed problematic. We previously used RightAction to mean the backward action (for group operation action), so we kept that part in the new alias and you have to explicitly use RightForwardAction. We are in the middle of making a breaking release anyway so we can clean this up but I'm not sure how.

[olivierverdier]

(Thank you all for your quick responses!) I would propose the following:

1. Keep LeftAction and RightAction as it was before, so

   struct LeftAction <: ActionDirection end
   struct RightAction <: ActionDirection end

2. Define

   abstract type GroupActionSide end
   struct LeftSide <: GroupActionSide end
   struct RightSide <: GroupActionSide end

3. Make GroupOperationAction dependent on the side, and define the corresponding Forward/Backward group actions.

   struct GroupOperationAction{G,AD,SD} <: AbstractGroupAction{AD}
   group::G
   end
   ForwardGroupAction(G::TM, ::TAD=LeftAction()) = GroupOperationAction{TM,TAD,LeftSide}(G)
   BackwardGroupAction(G::TM, ::TAD=LeftAction()) = GroupOperationAction{TM,TAD,RightSide}(G)

4. Keep backward compatibility (with perhaps a deprecation warning?)

   GroupOperationAction(G, ::LeftAction) = ForwardGroupAction(G, LeftAction())
   # possibly deprecate the following call
   @deprecate GroupOperationAction(G, ::RightAction)  BackwardGroupAction(G, RightAction())

5. switch_action_direction should switch left/right in a systematic way I believe this has to be done for each individual action?
6. Keep switch_direction as it is, but deprecate it, as in the future its behavior should be that of switch_action_direction?
7. switch_side could switch between the Forward/Backward actions, (only for group operation actions, otherwise it makes no sense)

   switch_side(::RightSide) = LeftSide
   switch_side(::LeftSide) = RightSide
   switch_side(A::GroupOperationAction{TM,TAD,RightSide}) = ...

8. Implement a default apply(A::AbstractGroupAction{RightAction},a,p) = apply(switch_leftright(A), inv(base_group(A),a), p)
9. Implement apply for Forward and Backward group actions
10. Remove the extra types ForwardBackwardSwitch, SimultaneousSwitch, etc., as well as the corresponding switch_direction implementations.

I believe that is not even breaking backward compatibility. Although, personnally, I would go ahead and break compatibility by removing the current switch_direction and replace it by the switch_action_direction above that always switches left/right.

What do you think?

[mateuszbaran]

I think it makes sense. I will try to do it as a part of #642 and see if there are any problems.

[olivierverdier]

And by @mateuszbaran s post RightAction() = RightBackwardAction() those two are equivalent by definition. So that is just a RightAction, because by definition they are equal, even in memory (that is ===).

Yes, I had missed that, thank you. But look at this simple example:

A = RotationAction(Euclidean(3), SpecialOrthogonal(3), RightAction())
A_ = switch_direction(A)

Now A_ is RotationAction(Euclidean(3; field = ℝ), SpecialOrthogonal(3), LeftBackwardAction()), and LeftBackwardAction() ≠ LeftAction(). I find this unnecessarily confusing.

(Actually, and it pains me to say that, 😭 I would even suggest to roll back this change (#639) for now. The change makes using Manifolds.jl just unnecessarily harder. The previous inconsistency at the root of this change was easier to get around. Thanks again to all of you for all the hard work! 🙏)

[mateuszbaran]

I don't think we can just roll back that change as it would be breaking. I'm going to reduce the scope of #642 a bit though to make it happen sooner. The rigid body dynamics part is very complicated so I will leave it out for later.

[olivierverdier]

For the record, the change #639 was utterly breaking (it certainly broke my code to smithereens 😀, as I am a heavy user of group actions). For anyone else bothered by that change, just check out v0.8.72 (e714d9d9) and stay there until this is all fixed.

[kellertuer]

I am not sure it was, since we usually care that we do not have to fix any tests. If it did break your code, we might have missed an inconsistency, or you did use parts differently from how we expected it to be used.

But what I currently read from this thread, it seems like you do not like the group parts here anyways. And sure, we are not-so-much group experts and the current active team of 2 developers can also only do a finite amount of work. Maybe doing that more thoroughly and in the long run do a separate LieGroups.jl package that depends on Manifolds.jl would be the best way to go – but again, the main limitation (besides knowledge on my side a bit) is time / people working on that.

[mateuszbaran]

Yes, I'm sorry that it takes so long but I'd like to fix this properly and bundle all breaking changes in one release. #642 is essentially in a polishing phase so hopefully it won't take too long. I think it's reasonable to keep using the old version in the meantime.

[mateuszbaran]

I just need to fix some inconsistencies in fiber bundles pointed out by @kellertuer and check docs & code coverage.

[olivierverdier]

Don't get me wrong, this library is amazing 🤩, and I absolutely ✨love✨ the Lie group part! Besides, as @mateuszbaran says, v0.8.72 works perfectly well. I am in no hurry at all, take your time fixing this. 😃

As to the backward compatibility breaking, here is a very concrete example (and this seriously breaks my code):

A = RotationAction(Euclidean(3), SpecialOrthogonal(3), RightAction())
A_ = switch_direction(A)
is_left = A_ isa RotationAction{<:Any, <:Any, LeftAction}

Now, is_left is true before v0.8.72, and it becomes false afterwards.

(I'm pretty sure this can be quickly fixed, but I would prefer a more robust solution to the underlying problem).

[kellertuer]

Thanks for the feedback – with so many errors reported at least I sometimes do confuse a lot of critical feedback with the toolbox not being good (so interprete the critical sometimes as negative – sorry for that). So yes, sorry, I understood you wrong there.

Concerning the error – I would not consider that breaking, because you rely heavily on the parameter types and their order of the RotationAction. Sure we should maybe start to export a few things less – but isa might be considered to already check against implementation detail and not API. So yes it does break things – but I would even say the better fix is to have accessors (is_left_action?) instead to abstract from such “hardcore type checking” code on a users side. So besides a more robust underlying solution, I would also prefer to have a nicer user API for such code requests you (or other users) need :)

[olivierverdier]

We may be misunderstanding each other again: I do not use isa in my code. (I don't think I'm doing "hardcore type checking" either, but I'm not sure, given its proximity to "multiple dispatch" 😄).

I just have functions that behave differently if given a left or a right action, and I use multiple dispatch for that.

Personal opinion: multiple dispatch should be allowed, and not considered as using an internal API. (Should I really be using if statements with functions like is_left_action instead? I hope not! 😅)

Finally, I'm totally, absolutely fine with breaking changes, as long as they make the library better. In my opinion, #639 didn't, hence my reaction.

Keep up the good work!! 💪

[kellertuer]

Ah, ok. Well then it might still be something one could consider having a function for to “hide implementation / design detail” or abstract from it. is_left_action should not be a function in the API that is correct, but in other cases it might be wort a discussion.

I am also fine with breaking changes, but it should not happen in patch versions (0.8.x) but only when increasing the middle SemVer number (while we are in the 0.x phase). That is what I am not so happy about, that it was breaking in a patch release.