Extend support for SpecialEuclidean, fiber bundles, optionally static sizes

[mateuszbaran]

One of the things I'm currently working on is rigid body dynamics using SE(n). I intend to add a bunch of missing features and a tutorial in this PR. The tutorial should potentially supersede #366 .

EDIT:
TODO:

• Check docs.
• Check coverage.
• Re-check Inconsistency with switch_direction? #637 new fix.
• Remove (?) LinearAffineMetric.

[codecov]

Codecov Report

Merging #642 (34fcb0a) into master (b03cabc) will decrease coverage by 4.54%.
The diff coverage is 92.55%.

@@            Coverage Diff             @@
##           master     #642      +/-   ##
==========================================
- Coverage   99.22%   94.68%   -4.54%     
==========================================
  Files         106      108       +2     
  Lines       10488    10562      +74     
==========================================
- Hits        10407    10001     -406     
- Misses         81      561     +480     

Files	Coverage Δ
ext/ManifoldsRecipesBaseExt.jl	97.61% <ø> (ø)
ext/ManifoldsTestExt/tests_general.jl	94.44% <100.00%> (-1.28%)	⬇️
ext/ManifoldsTestExt/tests_group.jl	99.73% <100.00%> (ø)
src/Manifolds.jl	86.66% <100.00%> (ø)
src/atlases.jl	93.58% <100.00%> (-1.29%)	⬇️
src/distributions.jl	66.66% <ø> (ø)
src/groups/GroupManifold.jl	100.00% <100.00%> (ø)
src/groups/addition_operation.jl	100.00% <ø> (ø)
src/groups/circle_group.jl	97.14% <100.00%> (-2.86%)	⬇️
src/groups/general_linear.jl	99.19% <100.00%> (+0.09%)	⬆️
... and 83 more

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mateuszbaran]

The last commit partially resolves #625 (I haven't updated all manifolds yet).

[kellertuer]

Since this one is breaking, could we base that on a breaking release of ManifoldsBase as well? Would that be a good reason to do that?

[mateuszbaran]

Resolving the exp/retract asymmetry would require a breaking release of ManifoldsBase.jl but I want to do things that don't require breaking Base first.

[kellertuer]

Ok, I was just thinking, since we are breaking something here, why not at the same time break ManifoldsBase as well, but that was also just an idea.

[mateuszbaran]

Documentation works now but normal tests seem to run out of memory on CI. I think we can cut some power or product manifold tests to help with that.

[kellertuer]

Sure, I mean some time after the TR rework I want to get back to minimising the tests anyways. So feel free to minimise the tests (as long as code coverage remains).

[mateuszbaran]

Tests are passing again and coverage is better than before 🎉

[mateuszbaran]

We have warnings about group.md being too big, so I've split actions to a separate page. Even after doing that, we still have a warning:

┌ Warning: Generated HTML over size_threshold_warn limit: manifolds/group.md
│     Generated file size: 166621 (bytes)
│     size_threshold_warn: 102400 (bytes)
│     size_threshold:      204800 (bytes)
│     HTML file:           manifolds/group.html

so in the future we may want to split it even further.

[kellertuer]

Yes we should probably split that – or rethink even the idea of doing a separate Lie groups package (i.e. define the manifolds parts here and the group “extensions” to said manifolds over there).

[kellertuer]

Overall this looks fine, just a few small remarks and three further points to discussion

• I feel the FiberBundleProductRetraction could have two retractions contained, one for the manifold part, one for the fiber part?
• the name direction_and_side sounds a bit strange
• For the new documentation like

  M::MetricManifold{ℝ,<:Stiefel{<:Any,ℝ},<:StiefelSubmersionMetric}

  it might be a bit hard to read in the docs compared to the old n,k in place of the <:Any I feel, since the n,k transported more information as well.

[mateuszbaran]

• I feel the FiberBundleProductRetraction could have two retractions contained, one for the manifold part, one for the fiber part?

Sure, that makes sense. Do you prefer to have it in this PR or could it be left for later?

• the name direction_and_side sounds a bit strange

It does but that function is just too useful and I don't have a better idea.

• it might be a bit hard to read in the docs compared to the old n,k in place of the <:Any I feel, since the n,k transported more information as well.

I think the best way to handle it is adding explanation about numbers like n and k where appropriate but I feel like many docstrings affected by this change didn't really lose that much.

[kellertuer]

• I feel the FiberBundleProductRetraction could have two retractions contained, one for the manifold part, one for the fiber part?

Sure, that makes sense. Do you prefer to have it in this PR or could it be left for later?

If it has a reasonable behaviour by now (does it map to both defaults by now?) then this is fine here but the more general case where one can even set them would be a reasonable extension for a next PR then.

• the name direction_and_side sounds a bit strange

It does but that function is just too useful and I don't have a better idea.

I do see that this is useful, I just feel the name is a bit strange, since it sounds like it would do two things at the same time.

• it might be a bit hard to read in the docs compared to the old n,k in place of the <:Any I feel, since the n,k transported more information as well.

I think the best way to handle it is adding explanation about numbers like n and k where appropriate but I feel like many docstrings affected by this change didn't really lose that much.

One could – if n and k are required in the doc string (I did not check all docs for this) add them either as a reminder or the math formula to specify n and k again.

[mateuszbaran]

If it has a reasonable behaviour by now (does it map to both defaults by now?) then this is fine here but the more general case where one can even set them would be a reasonable extension for a next PR then.

Currently it's only implemented for vector bundles and uses exp/+. That was sufficient so far. Anyway, I will work on extending it in a reasonably near future (probably after Manopt.jl/Optimization.jl integration).

I do see that this is useful, I just feel the name is a bit strange, since it sounds like it would do two things at the same time.

It kind of does two things 😅: returns both direction and side of group operation action, as a two-element tuple.

One could – if n and k are required in the doc string (I did not check all docs for this) add them either as a reminder or the math formula to specify n and k again.

Yes, something like that. If you had an idea for a copy-paste'able phrase I could quickly add, I could add it. Otherwise I'd prefer to leave that for later.

[kellertuer]

My main comment will be done in a future PR that is fine, for the direction and side I do not have a better idea and for the docs – maybe we can check carefully that they do not confuse users by now, but then we could also do documentation improvements in a next PR. So let's say this is fine now to not hold our ecosystem in an unstable state for too long (Manopt.jl will be next but hopefully an easy bump).

[mateuszbaran]

Great! I will wait for CI and merge it then.