Unify exp and retract, log and inverse_retract

kellertuer commented 10 months ago

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[bot] commented 10 months ago

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

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

kellertuer commented 10 months ago

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

[x] check and update documentation
[x] check test coverage.

mateuszbaran commented 10 months ago

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

mateuszbaran commented 10 months ago

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

kellertuer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 :slightly_smiling_face: . Other parts still need more work (by my current estimate they should be mostly ready by the end of September).

kellertuer commented 10 months ago

Great, will do.

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

mateuszbaran commented 10 months ago

Thanks!

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

kellertuer commented 9 months ago

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

mateuszbaran commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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!)

kellertuer commented 9 months ago

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

JuliaManifolds / ManifoldsBase.jl

Unify exp and retract, log and inverse_retract #167

Codecov Report