Introduce fallbacks for AbstractManifoldPoints and Vectors

[kellertuer]

While working on a second type for the Grassmann manifold, I endet up noticing that I would reimplement quite some stuff we did for Hyperbolic already, especially including the broadcast.
Also for a default type (for the Stiefel representation) on would reimplement all exp/log/...
so I thought one could also do this pass-through to the -value field by default such that a new point/tangent type has only to reimplement the stuff that is new/different.

This is currently done after all other stuff (bases / defaults) is done, i.e. on the lowest levels

I have not yet finished

• retractions / vector transports are missing
• tests are missing

but this would reduce quite some code in manifolds and omit quite some code I saw I would have to write next in Grassmann. What do you think?

[mateuszbaran]

I think I'd prefer a more granular approach with macros, like here for example:

macro manifold_point_forwards(T::Symbol, field::Symbol)
    return esc(quote
        allocate(p::$T) = $T(allocate(p.$field))
        allocate(p::$T, ::Type{P}) where {P} = $T(allocate(p.$field, P))
        function allocate(p::$T, ::Type{P}, dims::Tuple) where {P}
            return $T(allocate(p.$field, P, dims))
        end

        number_eltype(p::T) = typeof(one(eltype(p.$field)))
    end)
end

and so on for other methods.

Advantages:

1. Not all manifold points have to follow our defaults.
2. It's still easy to make a new type follow these defaults (basically writing @eval @manifold_point_forwards MyType value)
3. We can name the field differently in different types.
4. We can be more granular, for example while forwarding methods to the wrapped value is sometimes useful, it seems to me like a dangerous default behaviour, which in the case could be just written in a second macro and we could have different behaviours for different types.

[kellertuer]

Sure, that also sounds good. Just that I do not feel experienced enough to program that and usually macros are also quite complex. Can you mean help? And should we do that here or first over at manifolds?

[mateuszbaran]

Sure, I can write these macros. We can put some of them here, at least the part that would be useful to Validation manifold.

[codecov]

Codecov Report

Merging #89 (81e4d36) into master (7307caf) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head 81e4d36 differs from pull request most recent head c71b4c0. Consider uploading reports for the commit c71b4c0 to get more accurate results

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   99.85%   99.71%   -0.14%     
==========================================
  Files          14       15       +1     
  Lines        1339     1414      +75     
==========================================
+ Hits         1337     1410      +73     
- Misses          2        4       +2     

Impacted Files	Coverage Δ
src/ValidationManifold.jl	100.00% <ø> (ø)
src/vector_spaces.jl	100.00% <ø> (ø)
src/ManifoldsBase.jl	97.43% <100.00%> (-1.72%)	⬇️
src/bases.jl	100.00% <100.00%> (ø)
src/point_vector_fallbacks.jl	100.00% <100.00%> (ø)
src/DefaultManifold.jl	100.00% <0.00%> (ø)

---

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 7307caf...c71b4c0. Read the comment docs.

[mateuszbaran]

This should, more or less, work now. Macro @default_manifold_fallbacks should probably be in Manifolds.jl for now, I don't see much use for it here.

[kellertuer]

Thanks for the fast help; I think the main question for the fallbacks macro is: Is it useful as a tool that should stay here?

It is definite useful as a tool, so maybe it can stay here, since it would feel too heavy to load all of manifolds to just use it?

[mateuszbaran]

OK, it can also stay here. Let me know it you need some more help with this PR.

[kellertuer]

Great. I will check that the test coverage is given here while also using the macros already over at manifolds for the new type. Thanks for the help.

[kellertuer]

This looks good so far and I am now trying to use it with Hyperbolic first and then for the actual idea on ProjectionPoints in Grassmann – we just also have to increase test coverage here on the newly introduced functions that are now automatically generated.

[kellertuer]

I sketched the tests, but it seems the automatically introduced log! does not yet work.

Edit: I first thought one just has to fix the allocation type, but somehow adding that to the macro did not help, when calling it it does not return the new tangent vector type, which I hoped I had done, so I do currently not understand why the allocation fails (and hence the log does as well)

[mateuszbaran]

I'm just looking at it and it doesn't even reach log for me. I'll push a patch soon.

[kellertuer]

In my error it looks like It reaches log then allocates a DefaultPoint (but not a DefaultVector) and then dispatches to the wrong log! of course (since the macro one is with a vector ;) ) - and the wrong log! for sure can not do broadcast-thingies on the DefaultPoint. Here's the two interesting lines from the error stack (not the wrong type of Y).

    [6] log!(#unused#::DefaultManifold{Tuple{3}, ℝ}, Y::DefaultPoint{Vector{DefaultTVector}}, p::DefaultPoint{Vector{Float64}}, q::DefaultPoint{Vector{Float64}})
      @ ManifoldsBase ~/Repositories/Julia/ManifoldsBase.jl/src/DefaultManifold.jl:94
    [7] log(M::DefaultManifold{Tuple{3}, ℝ}, p::DefaultPoint{Vector{Float64}}, q::DefaultPoint{Vector{Float64}})
      @ ManifoldsBase ~/Repositories/Julia/ManifoldsBase.jl/src/exp_log_geo.jl:73

[kellertuer]

As mentioned above we could also add retractions and inverse retractions and the vector transport to that macro, if that does not destroy the rest of the world (maybe it does ;) ).

[mateuszbaran]

Let's fix the current code first... I've just noticed that I have forgotten to pull new changes which caused the problem of not reaching log. Now I'm fighting with the log problem itself.

[mateuszbaran]

I've fixed a bunch of cases. Currently the one that fails is zero_vector but I'm not sure what the right solution is in this case.

[kellertuer]

I would say similar to the log!– allocate the corresponding $TV and define zero_vector!.

[mateuszbaran]

Maybe it would make more sense to unwrap, call zero_vector on the unwrapped point and wrap the result?

[kellertuer]

Oh, that sounds really reasonable as well!

[mateuszbaran]

I've fixed that vector problem and some more. Now the problem is with embed! and that MatrixVectorTransport thing which I forgot what it actually does. Do you have some ideas about these?

[kellertuer]

The MatrixVectorTransport just models a curve – creating it correctly for the DefaultPoint solved that point.
I also fixed embed! and introduced the vector transports, retraction and inverse retraction (and resolved their ambiguities with exp/log). Now (hopefully) just the vector transport ambiguities might be missing.

[kellertuer]

Maybe we can resolve the vector transport ambiguities similar to how you resolved the vector/coordinate functions?

[mateuszbaran]

I think so, I'll try doing that today.

[kellertuer]

I had a moment and extended test coverage a little bit, but

• retract!
• inverse_retract!
• vector_transport_to!
• vector_transport_direction!
• a few lines in the broadcast

are not covered yet. For the first 4 I ran into Ambiguities again, I left the two for retract and inverse retract in (as broken tests) but I have no good idea yet how to resolve them.

[mateuszbaran]

retract! and inverse_retract! ambiguities would be solved by the two-level scheme since DefaultPoint unwrapping would be at the top level and actual implementation at the lower level. Currently there is no other way than manually resolving such ambiguities.

[kellertuer]

I see, maybe than it is not worth having these in the macro?

edit: to be precise neither retract! nor inverse_retract! nor the vector_transort_X!, since they all require a manual resolving anyways.

[mateuszbaran]

Yes, I've removed them.

[kellertuer]

Oh noes we increased the not-covered lines by 50% (2-3) – we could check for those maybe, but in general I think this PR should work for now :)

Or the analysis was not yet finished, since in the web interface of codecov it still reports 2 lines (and no missing line for the PowerManifold).

[mateuszbaran]

The new non-covered line is, I think, a false positive, there is nothing we can do about it.

[mateuszbaran]

I'll review it once again sometime soon but this indeed seems to be complete.

[kellertuer]

When I find time (during exam/corrections time and preparing for the new semester starting January 10) I will apply these to the Projector-Grassmann-branch I started with before this detour ;)