Unify exp and retract, log and inverse_retract

[kellertuer]

The unification concerns the “dispatch flow”

Now both

• allocate first, dispatch on eat ! method
• exp and retract fuse t and X “last”

This way

1. a performance aware user can implement the t, X variant, otherwise the fused version with just X is enough to implement
2. We only have a layer-2-dispatch-tree for the inlace variant; this reduces code and is nice for the user, since when they want to implement the allocating variant, they anyways have the specific M and method types, such that there should not be problems with ambiguities.

This is till WIP, since I have to check that all tests are fine till and a bit that manifolds.jl also still works fine ;)
This resolves #159.

[codecov]

Codecov Report

Merging #167 (b37bb31) into master (5e7f6ae) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files          26       26              
  Lines        3160     3063      -97     
==========================================
- Hits         3159     3062      -97     
  Misses          1        1              

Files	Coverage Δ
...rayToolsExt/ManifoldsBaseRecursiveArrayToolsExt.jl	100.00% <ø> (ø)
...yToolsExt/ProductManifoldRecursiveArrayToolsExt.jl	100.00% <ø> (ø)
src/point_vector_fallbacks.jl	100.00% <100.00%> (ø)
src/retractions.jl	100.00% <100.00%> (ø)
src/shooting.jl	100.00% <ø> (ø)
src/vector_transport.jl	100.00% <ø> (ø)

... and 2 files with indirect coverage changes

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

[kellertuer]

I even found a small bug where some vector fall back of retract accidentally returned X instead of q (though it took me an hour to narrow it down).

Good points:

• The tests are running fine again
• the amount of code is reduced drastically
• one can still define individual retractions – just one has to be precise in parameter types if one skips a layer (and for the allocating ones these layers no longer exist, so one has to be precise)
• For Manopt this update is non-breaking
• For Manifolds.jl it seems to break only some of the number-manifolds, where we used the now-removed retract-layers (Circle, Euclidean, and one in interaction with SPDPoint - where just an allocation needs to be fixed. Tangent vectors are classical matrices and not SPDPoints)

But besides the small break on allocating manifolds, this seems to not only unify code but also simplify our code base!

Still ToDo

• check and update documentation
• check test coverage.

[mateuszbaran]

I think this looks quite good. It looks like updating Manifolds.jl later won't be much of a problem 🙂 .

[mateuszbaran]

BTW, if we make a breaking release of ManifoldsBase.jl maybe you could also propose a change to #119 that you wanted? Ref. JuliaManifolds/Manifolds.jl#438

[kellertuer]

Sure, will recheck the test tomorrow and yes, we can surely also introduce an inverse retraction based distance approximation, just that I am missing a neat name for it.

[kellertuer]

I thought a bit about the arguments with distance, the main problem I have is, that I do not have a good idea for a name. So we could consider distance as high-level as e.g. the algorithms in Manopt.jl and keep its name and the argument (though I said something different before); just because if there is no good name, then there is no use in renaming it.

[kellertuer]

@mateuszbaran you introduced a specific function definition once for mid_point (and only that) to work on zero-dimensional arrays on Julia pre 1.1.

None of the tests ran through that, I even tried to trigger this function in a Julia 1.0 session but could not. I removed it in the last commit. Can you check that this is ok or otherwise write a test to cover that case?

[mateuszbaran]

IIRC it was due to some bug on Julia 1.0. I think that method could be removed and Julia requirement bumped to 1.6 here?

[kellertuer]

Since it was a mid_point! function we did not loose coverage due to my rework (it might have skipped a few allocating ones due to the allocate first strategy), so my interpretation is, that the bug might be fixed.
What du you think?

But I am also fine to use this breaking change to remove support for 1.0 – through it is nice to support the interface even on very old versions, none of our other packages supports 1.0 anymore, either.

[mateuszbaran]

ManifoldsBase.jl isn't very useful without other libraries so I think it's fair to remove support for 1.0. After all, old versions that work with 1.0 will still be available.

[kellertuer]

Ok, been there, done that.

Should we merge this and wait for a few more breaking things before registering 0.15? I could work on the metric manifold next (just have to remember, where I stopped and why).

[mateuszbaran]

I wouldn't merge it until we are closer to releasing Manifolds.jl 0.9. Maybe after I finish and you review the key parts of my PR? We don't really need this merged earlier, and merging it will just make tagging new versions of ManifoldsBase.jl 0.14.x more complicated.

[kellertuer]

Ok, sounds fair.
If we have a few of these, we could think of an interims dev-0.15 branch, but you are right for one PR we can also just wait.

I can check your PR soon-ish – but for now it is still a draft, right?

[mateuszbaran]

Yes, my PR is still a draft but some parts are more or less finished. I think you can check the changes related to optional static size and removal of ProductRepr. It's really just one pattern applied to all manifolds which should be relatively easy to review and would cover a lot of code affected by that PR. I've also started a NEWS file which you can also check 🙂 . Other parts still need more work (by my current estimate they should be mostly ready by the end of September).

[kellertuer]

Great, will do.

Did you see that I added the TypeParameter to the docs here? Please check and improve if I missed something :)

[mateuszbaran]

Thanks!

I haven't checked those docs yet, but I will 🙂 .

[kellertuer]

This was reasonably easy to merge. Let me know when you have checked this on Manifolds.jl

[mateuszbaran]

Something broke with Circle. inverse_retract(M::Circle{ℝ}, p::Float64, q::Float64, m::LogarithmicInverseRetraction) used to work but now it tries to allocate (which obviously fails). Shouldn't it forward to log instead of inverse_retract!?

[mateuszbaran]

There is a similar issue with retract, for example retract(M::Euclidean{Tuple{}, ℝ}, p::Float64, X::Float64, m::ExponentialRetraction) calls retract! instead of exp.

[kellertuer]

Ah interesting, that is a case we did not take into account. But then maybe we have to reintroduce _retract that dispatches on the method (either passing to exp or allocating and to retract!)

[mateuszbaran]

I've adapted Manifolds.jl PR to this change and tests are passing.

[kellertuer]

Cool! Then I will merge this now into main – then merge main into the last PR :)