Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lie_bracket and adjoint_action #381

Merged
merged 45 commits into from
Jul 13, 2021
Merged

lie_bracket and adjoint_action #381

merged 45 commits into from
Jul 13, 2021

Conversation

mateuszbaran
Copy link
Member

This PR adds two things that seem to be missing from our Lie group/Lie algebra support, as identified in #366 .

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #381 (2c604ff) into master (8ac6857) will increase coverage by 0.03%.
The diff coverage is 97.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   97.73%   97.77%   +0.03%     
==========================================
  Files          76       78       +2     
  Lines        5876     6060     +184     
==========================================
+ Hits         5743     5925     +182     
- Misses        133      135       +2     
Impacted Files Coverage Δ
src/Manifolds.jl 100.00% <ø> (ø)
src/groups/semidirect_product_group.jl 100.00% <ø> (ø)
src/groups/group.jl 90.29% <84.21%> (-0.47%) ⬇️
src/tests/tests_group.jl 94.72% <97.29%> (+0.34%) ⬆️
src/manifolds/ConnectionManifold.jl 98.18% <98.18%> (ø)
src/groups/array_manifold.jl 100.00% <100.00%> (ø)
src/groups/circle_group.jl 100.00% <100.00%> (ø)
src/groups/connections.jl 100.00% <100.00%> (ø)
src/groups/special_euclidean.jl 100.00% <100.00%> (ø)
src/manifolds/MetricManifold.jl 97.05% <100.00%> (-0.36%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ac6857...2c604ff. Read the comment docs.

@kellertuer
Copy link
Member

We might need some more documentation, especially, it would be nice to have the general case ( https://en.wikipedia.org/wiki/Lie_bracket_of_vector_fields ) on Manifolds, too (at least have it in mind here).

@mateuszbaran
Copy link
Member Author

Yes, I'll add more documentation here. For this PR I'm going to restrict the Lie bracket to Lie algebras, and the more general case can be added later (I don't even know what could it be useful for).

Copy link
Member

@sethaxen sethaxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! It may be the case that the approach for SpecialEuclidean works more generally for semidirect product groups. i.e. the normal subgroup's part of the lie bracket looks like the lie bracket of the normal subgroup. We'd have to think more carefully about the other part of the tangent vector though.

src/groups/circle_group.jl Outdated Show resolved Hide resolved
src/groups/circle_group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/group.jl Outdated Show resolved Hide resolved
src/groups/special_euclidean.jl Outdated Show resolved Hide resolved
src/groups/special_euclidean.jl Outdated Show resolved Hide resolved
@mateuszbaran
Copy link
Member Author

Looks good! It may be the case that the approach for SpecialEuclidean works more generally for semidirect product groups. i.e. the normal subgroup's part of the lie bracket looks like the lie bracket of the normal subgroup. We'd have to think more carefully about the other part of the tangent vector though.

There is such a thing as a semidirect sum of Lie algebras but it looks like in practice the structure can be most easily obtained by embedding is some higher dimensional general linear group. There are some more general details here: https://en.wikipedia.org/wiki/Lie_algebra_extension#By_semidirect_sum .

Co-authored-by: Seth Axen <seth.axen@gmail.com>
@sethaxen
Copy link
Member

There is such a thing as a semidirect sum of Lie algebras but it looks like in practice the structure can be most easily obtained by embedding is some higher dimensional general linear group. There are some more general details here: https://en.wikipedia.org/wiki/Lie_algebra_extension#By_semidirect_sum .

Yeah that looks identical to what we see in the special case of semidirect products embedded in GL(n+1) with the affine representation, it just has the extra Lie bracket for the non-normal subgroup, which produces zero vectors in the case where we can use the affine representation. Seems like a good default to have.

mateuszbaran and others added 2 commits June 14, 2021 09:52
Co-authored-by: Seth Axen <seth.axen@gmail.com>
src/groups/group.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mateuszbaran
Copy link
Member Author

I've read a bit about the representation theory of the adjoint representation and it is very nontrivial. I am no longer sure this is the right design 😕 .

@sethaxen
Copy link
Member

I've read a bit about the representation theory of the adjoint representation and it is very nontrivial. I am no longer sure this is the right design 😕 .

What in particular are your concerns?

@mateuszbaran
Copy link
Member Author

What in particular are your concerns?

Mostly the fact that adjoint representations of many groups and algebras are not faithful, and that we still won't have any generic faithful representations that would work on FVectors (if that is even possible?).

@mateuszbaran
Copy link
Member Author

OK, after thinking a bit more about it I think these methods can be useful anyway. I'd still like to finish differentials of translations and adjoint action on SE(n) (primarily SE(3)).

@mateuszbaran
Copy link
Member Author

@sethaxen Where did you get the formula for function translate_diff!(G::SemidirectProductGroup, Y, p, q, X, conX::LeftAction) from? I'm trying to understand it and I'm actually surprised that we can't for some reason get translate_diff for SE(n) through embedding it in GL(n+1)? Your code gives a different translation part of the returned tangent vector.

@sethaxen
Copy link
Member

@sethaxen Where did you get the formula for function translate_diff!(G::SemidirectProductGroup, Y, p, q, X, conX::LeftAction) from? I'm trying to understand it and I'm actually surprised that we can't for some reason get translate_diff for SE(n) through embedding it in GL(n+1)? Your code gives a different translation part of the returned tangent vector.

I don't remember or if I worked it out myself. Can you share a snippet that fails?

@mateuszbaran
Copy link
Member Author

I guess nearly any invocation of translate_diff would differ between your version and the embedding in GL(n+1) but here is what I'm currently using for testing:

julia> using Manifolds, LinearAlgebra

julia> G = SpecialEuclidean(3);

julia> t = Vector{Float64}.([1:3, 2:4, 4:6]);

julia> ω = [[1.0, 2.0, 3.0], [3.0, 2.0, 1.0], [1.0, 3.0, 2.0]];

julia> p = Matrix(I, 3, 3);

julia> Rn = Rotations(3);

julia> pts = [ProductRepr(ti, exp(Rn, p, hat(Rn, p, ωi))) for (ti, ωi) in zip(t, ω)];

julia> X = ProductRepr([-1.0, 2.0, 1.0], hat(Rn, p, [1.0, 0.5, -0.5]));

julia> q = ProductRepr([0.0, 0.0, 0.0], p);

julia> translate_diff(G, pts[1], q, X)
ProductRepr with 2 submanifold components:
 Component 1 =
  3-element Vector{Float64}:
   2.2112553975587987
   0.5176292379367051
   0.9178287088559302
 Component 2 =
  3×3 Matrix{Float64}:
    0.0  0.5   0.5
   -0.5  0.0  -1.0
   -0.5  1.0   0.0

julia> X
ProductRepr with 2 submanifold components:
 Component 1 =
  3-element Vector{Float64}:
   -1.0
    2.0
    1.0
 Component 2 =
  3×3 Matrix{Float64}:
    0.0  0.5   0.5
   -0.5  0.0  -1.0
   -0.5  1.0   0.0

When calculating things using the embedding in GL(n+1) the left translate_diff would leave X unchanged.

@sethaxen
Copy link
Member

I wonder if maybe the difference is the choice in representation. For GL(n+1), all tangent vectors X at point p are represented instead as p \ X. And while SO(n) adopts the same convention SE(n) does not, representing instead the rotational part of the tangent vector as R^T Ω but doing nothing to the translational part.

@mateuszbaran
Copy link
Member Author

That might be it. Why did you choose that convention? I think having semidirect products easily identifiable as subgrups of GL(M) would make defining new ones much easier and consistent, and this convention makes this identification hard.

@sethaxen
Copy link
Member

That might be it. Why did you choose that convention?

Because it involved less divisions by points for LeftAction, which would probably be the most commonly used one. Otherwise almost every operation with tangent vectors uses division by a point. For SL(n) it also makes checking the validity of a tangent vector easier/cheaper.

src/groups/rotation_action.jl Outdated Show resolved Hide resolved
src/groups/rotation_action.jl Outdated Show resolved Hide resolved
src/groups/semidirect_product_group.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
mateuszbaran and others added 5 commits July 6, 2021 15:21
Co-authored-by: Ronny Bergmann <git@ronnybergmann.net>
Co-authored-by: Ronny Bergmann <git@ronnybergmann.net>
@mateuszbaran
Copy link
Member Author

I think I've addressed all issues now, except the SE_in_GL which I don't know what to do about -- it looks like a great use case for EmbeddedManifold but there seems to be no nice name for it.

@kellertuer
Copy link
Member

What do we do about the vector transports that are documented for identity but implemented more general? I hope I have marked all resolved questions as resolved, so could you once more check the ones I did not yet mark?

For the name: What about SpecialEuclideanInGeneralLinear? Maybe a little too long but I thin nicer than the short form?

@mateuszbaran
Copy link
Member Author

I've improved documentation of vector transports today, is it still inconsistent somewhere? The currently unresolved comments are:

  1. This: lie_bracket and adjoint_action #381 (comment) where the conclusion seems to be that the current design is OK.
  2. Here: lie_bracket and adjoint_action #381 (comment), lie_bracket and adjoint_action #381 (comment), and this: lie_bracket and adjoint_action #381 (comment) which should be (I hope) resolved by changes I've pushed today.
  3. Here: lie_bracket and adjoint_action #381 (comment), and I'll rename that constant to SpecialEuclideanInGeneralLinear.
  4. lie_bracket and adjoint_action #381 (comment) and there: lie_bracket and adjoint_action #381 (comment) I've added references today, and I think it should be fine to have some explicit non-mutating variants for the special Euclidean, special orthogonal and translations groups for better interoperability with SArray and AD.

@kellertuer
Copy link
Member

Thanks for the summary, the ones where the line got changed too much did not show up in the “Files Changed” view so I must have missed them that way; I only have the one open one left I just commented on. Thanks for carefully working on all my remarks.

mateuszbaran and others added 2 commits July 9, 2021 16:52
Co-authored-by: Ronny Bergmann <git@ronnybergmann.net>
@mateuszbaran
Copy link
Member Author

Thanks for a review. This is a quite significant set of changes so it's good to have them checked 🙂.

@kellertuer
Copy link
Member

Once the tests have passed, I think this can be merged, though we loose two lines in coverage (if you want, you could check for these) but it is within the limit of our tolerance.

@mateuszbaran
Copy link
Member Author

Great! These two lines reported as not covered look like false positives to me so I'd ignore that.

@mateuszbaran
Copy link
Member Author

Julia 1.4 CI seems broken 😕

@kellertuer
Copy link
Member

It seems that the call to ambigs = Test.detect_ambiguities(Base) does not finish in the our_base_ambiguities() function (tested locally on my machine with Julia 1.4. Commenting that base ambiguities test out, the test here locally run fine on Julia 1.4

@mateuszbaran
Copy link
Member Author

Weird, from my tests it looks like DoubleFloats alone stops ambiguity detection from completion. I'll move this test to newer versions and bump the number of ambiguities accordingly.

@mateuszbaran
Copy link
Member Author

Well, on 1.6 with all the other packages loaded it also takes way too long.

@mateuszbaran mateuszbaran merged commit 10d264d into master Jul 13, 2021
@kellertuer kellertuer deleted the mbaran/lie-bracket branch August 2, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants