Simplify MetricManifold to not use SimpleTraits

[kellertuer]

While working on a first example on a manifold with two different metrics in #27 I noticed that quite a few things are nice, with metric manifold but with the hasMetric trait it has a mayor drawbag:

All functions that currently have a fallback from MetricManifold{M,G} for the hasMetric{M,G} (let's call that default metric for now) trait have the large disadvantage, that every new and non default metric has to implement every function that has a fallback implemented. Also the fallbacks are all quite longish.

I propose the following:

• Remove the hasMetric trait and all hasMetric fallbacks
• Introduce conversions to the non MetricManifold case for the default metric and all metrics that should fall back to default implementations for certain functions
• these conversions do not “trigger” if one explicitly overwrites for example exp(MetricManifold{M,G},x,v) for a new metric G (but would trigger for manifold_dimension if that's not explicitly implemented for just mentioned type
• keep the !hasMetric implementations of for example exp!, flat!, and sharp! in MatricManifold, since if you implement a new metric, you'd implement these anyways ( or let them fall back)

I will try this today on #27, but of course would love to hear other opinions on that, too.

[kellertuer]

And a small addendum: why are flat and sharp written for FVectors of two types an not for TVector and CoTVector ? Do flat and sharp need typing there anyways, since it's the defaults for MetricManifolds?

[mateuszbaran]

IIRC it was argued that along with the introduction of ManifoldsBase.jl, metric manifolds should be moved to THTT. I think this should make it possible to have fallbacks that don't have to be implemented for every new metric. Essentially I don't see the point of having the !hasMetric methods in the first place, they could just be the default fallback.

And a small addendum: why are flat and sharp written for FVectors of two types an not for TVector and CoTVector ? Do flat and sharp need typing there anyways, since it's the defaults for MetricManifolds?

A few reasons:

1. Semantics of TVector and CoTVector suggest that they will be checked using but FVector leaves this unspecified.
2. flat and sharp can be defined for other vector bundles.
3. Not wrapping arguments in anything would make it too easy to put them in the wrong order when calling.

[kellertuer]

Thanks for the explanation on the FVectors – you're right, with the model assumption, that TVector and CoTVector are type enhancements merely for checking e.g. validity, FVector is the right choice.

I hope that omitting the trait and maybe doing THTT yields a nice default fallback idea. Let's keep this open and start a PR after transition to ManifoldsBase. For now I got #27 to work (with a little too many explicit fallbacks) and we can refactor afterward that is merged.

[kellertuer]

I found a way interims to THTT that already simplifies a lot of code, but leaves 2-3 lines in SymmetricPositiveDefinite.jl that are not so nice (explicit fallbacks), that can be optimised after the transition to THTT.

[kellertuer]

As a first step, I started a branch but I am again stuck in with a small detail on.
Look at this line
and the BaseManifold from here and M,g,MM afterwards as well as the setting of the default...

But then calling

convert(BaseManifold,MM)

does not work since one actually needs

convert(BaseManifold{3},MM)

to work. Can that be tricked in somehow?

I did not yet open a PR, since convert for the log does not yet work I fear (it still says that for MM there is no log instead of using the one for M) – and that would be really handy to have that for default metrics by covert (in my mind).

[mateuszbaran]

I think changing

convert(::Type{MT},M::MetricManifold{MT,GT}) where {MT <: Manifold,GT} = base_manifold(M)

to

convert(::Type{MT},M::MetricManifold{<:MT,GT}) where {MT <: Manifold,GT} = base_manifold(M)

should make convert(BaseManifold,MM) work.

[kellertuer]

Cool. Still 3 things that I think should convert, to not (namely I removed the explicit definitions of removing the metric for log!) for example and I had hoped that
(again from the metric test) log!(MM,x,y) would with convert get log!(base_manifold(M),x,y),
I have to check whether that can be done that way...

[mateuszbaran]

One thing that you have to do is modifying a bit default implementations of functions on manifolds so that base_manifold can be called there.
Similarly to

manifold_dimension(M::Manifold) = manifold_dimension(M, is_decorator_manifold(M))
manifold_dimension(M::Manifold, ::Val{true}) = manifold_dimension(base_manifold(M))
manifold_dimension(M::Manifold, ::Val{false}) = error("manifold_dimension not implemented for manifold $(typeof(M)).")

in ManifoldsBase.jl, all other functions have to be modified this way. All functions where you want base_manifold to be automatically called.

[kellertuer]

I had hoped to avoid that with a trick, since this would mean that we have to do that for every manifold function where we think the metric might play a role

for manifold_dimension, since this is independent of a metric

manifold_dimension(M::Manifold, ::Val{true}) = manifold_dimension(base_manifold(M))

is enough, but for exp!, log!, inner this would be required as long as distance and norm for example have good default fallbacks.

Edit: I think I know how to do it. If you don't specify anything, MetricManifolds fall back to the functions by @sethaxen – if you specify a metric as default they fall back to the ones of the corresponding Manifold, without metric.
...and hence if you then know better you implement – for example exp! for your specific MetricManifold anyways. I will model (and document) this after my teaching today :)

[kellertuer]

So the only things left, that either I get confused on or Julia (or both) is, that I get a few errors, where Julia can not find a function since on argument is a ::Type{Val{true}} instead of a ::Val{true} – and I don't see where I am doing what wrong for that. But at least during all those tests I also wrote a documentation for the new documenter approach and I will open the PR (with WIP) now already.

Edit: Found it! I was confused and returned Val{true} (that is the type!) instead of Val(true) in the test where I defined the defaultManifold.