Sketch change metric.

[kellertuer]

This PR is a step towards #17.

the idea here is as follows

change_metric (currently just its doctoring, which locally failed to build, let‘s see – I seem to miss something maybe for the docs?) should change the metric, i.e. in a basis compute Z = G_1^-1G_2 X (to switch X from G2 to G1).

Signatures are

Z = change_metric(M::AbstractManifold, G2::AbstractMetric, p, X)

or

Z = change_metric(M::MetricManifold{M,G1}, G2::AbstractMetric, p X)

And it would convert into the metric of the first given manifold. From the embedding, e.g. Euclidean with EuclideanMetric() one would call for example

Z = change_metric(SymmetricPositiveDefinite(n), EuclideanMetric(), p, X)

In order to get this into something like the discussed egrad2rgrad I would propose a function convert_tangent(M, N, p, X) where N is the embedding (if M itself is not an EmbeddedManifold)

1. First call project(M, p, X)
2. For the second I am unsure: First change representation? First change metric? Representation info what? For FixedRankMatrices I think we first need a conversion into the 3-factor TMVVector – but what if the user wants a different representation? How would we distinguish these? Or would this already be done by project? Maybe with an optional parameter indicating the representation type (e.g. which of the three for Hyperbolic)?
3. call change_metric as sketched here
4. For some manifold we have to “transfer” this correct tangent vector to the right representation, especially for Lie groups, we would parallel transport in the identity (i.e. Lie algebra)

I think we could provide a default change_tangent that just performs steps 1 and 3, and this would be overwritten, e.g. FixedRankMatrices would perform 2 (split into T M V before the metric change) and Lie groups would by default perform 1,3, and 4.

Similarly, with a different change_metric for matrix operations on the tangent space (or actions of matrices, so 1-1 and 2-0 tensors in comparison) some convert_matrix would be a similar approach to generalise ehess2rhess

Adendum: So if one has the Euclidean gradient grad = p -> ... by some machinery, the Riemannian gradient would – with this sketch be p -> convert_tangent(M, p, grad(p)) (if M has an implicit default embedding).

Adendum2: Of course convert_tangent_ is not yet the nicest of names, I think, though it converts a Euclidean (or in the embedding) tangent vector to a Riemannian one. Other ideas are welcome.

[kellertuer]

For the documentation - I do not understand why it currently complains

 LoadError: UndefVarError: change_metric not defined

I document the function signature once and export it – as we do for some functions that are first only given in signature. Hm. Maybe I am overlooking something since it is getting late.

[mateuszbaran]

For the documentation - I do not understand why it currently complains

 LoadError: UndefVarError: change_metric not defined

I document the function signature once and export it – as we do for some functions that are first only given in signature. Hm. Maybe I am overlooking something since it is getting late.

I don't understand it either. Documenter sometimes has weird problems with building PR docs.

[kellertuer]

I can get the same error locally, so this is not only on PR ;)

[mateuszbaran]

That's a really weird error... maybe environment in which docs are built doesn't have Manifolds.jl correctly deved? I'm not sure.

[kellertuer]

That's a really weird error... maybe environment in which docs are built doesn't have Manifolds.jl correctly deved? I'm not sure.

I do not know. Locally it just gets weirder

ERROR: LoadError: SystemError: opening file "/var/folders/q4/7mgqym9s5nx0kq4jx_z40wj00000gn/T/jl_me64AL.png": No such file or directory

whatever. I‘ll lave it for now trying to build to docs, I think its GR again. GR sometimes does very strange things.

[kellertuer]

I thought a little bit about steps 2 (changing type) and 4 (adaption to local tangent vector representation).
The order of 3 and 4 is irrelevant I think, so 4 could be after 2.

Then I think both 2 and 4 should be done in project for the following reason: If I project (with the metric from the embedding) onto a manifold, I would expect is_vector(M, p, project(M, p, X)) to return true. For manifolds like fixed rank this requires the right type (that is project should do 2) and for Lie groups it requires an element from the Lie algebra (that is project should do 4).

I am currently updating the docs to the result from our discussion, but locally I can not build the docs and look why it fails here on PR, since JuliaPlots/Plots.jl#3800 either Plots or GR is just completely broken currently.

[kellertuer]

Just when I thought I had understood what we are doing, I think I again do not understand what we are doing. See
https://github.com/NicolasBoumal/manopt/blob/3774b069666a3a1a01a33e10f53062e33495e59e/manopt/manifolds/symfixedrank/sympositivedefinitefactory.m#L102

There the transportation of a tangent vector X at p (there notation is \xi and X) gets transformed as Z = pXp.
I would have just expected pX from the discussion above since the equation reads that we are looking for a transformation B such that (g_E the euclidean we are coming from g_p the linear affine

g_E(X,Y) = tr(XY) = g_p(BX,BY) = tr(p^{-1}BXp^{-1}BY)

Here clearly this yields B=p and not the equation above. So somehow the change_metric I just updated the documentation to and the linked conversion (the symm inside should just be the projection) – are still not the same, so just change_metric after projection (type change and parallel transport to I) – is not what Manopt is doing, they somehow more look at

g_E(X,Y) = tr(XY) = g_p(f(X),B) = tr(p^{-1}Xp^{-1}Y)

for fixed X (and all Y) what I started here with, too? because then, sure, f(X) = pXp. Or am I missing something? So what is now the actual conversion we are looking for and why?

[codecov]

Codecov Report

Merging #423 (4501a27) into master (bd2b425) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   97.96%   97.99%   +0.02%     
==========================================
  Files          79       79              
  Lines        6501     6585      +84     
==========================================
+ Hits         6369     6453      +84     
  Misses        132      132              

Impacted Files	Coverage Δ
src/Manifolds.jl	100.00% <ø> (ø)
src/groups/group.jl	91.33% <ø> (ø)
src/manifolds/Multinomial.jl	100.00% <ø> (ø)
src/manifolds/Spectrahedron.jl	100.00% <ø> (ø)
src/manifolds/CenteredMatrices.jl	100.00% <100.00%> (ø)
src/manifolds/GeneralizedGrassmann.jl	100.00% <100.00%> (ø)
src/manifolds/Hyperbolic.jl	100.00% <100.00%> (ø)
src/manifolds/HyperbolicHyperboloid.jl	100.00% <100.00%> (ø)
src/manifolds/HyperbolicPoincareBall.jl	100.00% <100.00%> (ø)
src/manifolds/MetricManifold.jl	97.93% <100.00%> (+0.87%)	⬆️
... and 7 more

---

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 bd2b425...4501a27. Read the comment docs.

[kellertuer]

After a short question to the Manopt forum and some further thinking I found the issue at least for the gradient:
We do not want to change the metric (though having https://juliamanifolds.github.io/Manifolds.jl/previews/PR423/manifolds/metric.html#Manifolds.change_metric-Tuple{AbstractManifold,%20AbstractMetric,%20Any,%20Any} is surely nice) but we want to switch the Riesz representer of the differential (since we switch the metric). I documented this at https://juliamanifolds.github.io/Manifolds.jl/previews/PR423/manifolds/metric.html#Manifolds.change_tangent-Tuple{AbstractManifold,%20AbstractMetric,%20Any,%20Any} – where I do not like the name yet.

Note that this requires one representation of tangent vectors, that is the shift to the Lie algebra can happen before or after, since it is not important at which point we switch the metric (for neither of those functions).

What remains is to decide for proper names (one could call the second also change_representer or something) and decide where to put the type-conversion and the parallel transport to the identity. I am still for doing both in project.
Nicolas also wrote here https://groups.google.com/g/manopttoolbox/c/eiuq_BfENjU that this method should work for quotient manifolds with total space being Euclidean as well – I think it even works between arbitrary manifolds as long as there is a projection possibility.

[mateuszbaran]

I think I'm starting to see how it's supposed to work, in particular why you did change_metric that way initially.

https://juliamanifolds.github.io/Manifolds.jl/previews/PR423/manifolds/metric.html#Manifolds.change_tangent-Tuple{AbstractManifold,%20AbstractMetric,%20Any,%20Any} – where I do not like the name yet.

I think a name like change_gradient_metric or something like that would be fine.

Note that this requires one representation of tangent vectors, that is the shift to the Lie algebra can happen before or after, since it is not important at which point we switch the metric (for neither of those functions).

My first thought is to require gradients in the correct representation to be passed to change_gradient, so switching representation would happen first. Maybe it's fine to do it the other way too but it's not obvious to me.

What remains is to decide for proper names (one could call the second also change_representer or something) and decide where to put the type-conversion and the parallel transport to the identity. I am still for doing both in project.

It seems reasonable to put both type conversion and parallel transport in project. By the way, vector projections of Rotations manifold doesn't do parallel transport currently as far as I can tell, it's a different vector transport.

Nicolas also wrote here https://groups.google.com/g/manopttoolbox/c/eiuq_BfENjU that this method should work for quotient manifolds with total space being Euclidean as well – I think it even works between arbitrary manifolds as long as there is a projection possibility.

Great, that's quite helpful 🙂 .

[kellertuer]

https://juliamanifolds.github.io/Manifolds.jl/previews/PR423/manifolds/metric.html#Manifolds.change_tangent-Tuple{AbstractManifold,%20AbstractMetric,%20Any,%20Any} – where I do not like the name yet.

I think a name like change_gradient_metric or something like that would be fine.

That sounds like a reasonable name, I will think whether it can have a more general name even, since this might not only be used for gradients.

Note that this requires one representation of tangent vectors, that is the shift to the Lie algebra can happen before or after, since it is not important at which point we switch the metric (for neither of those functions).

My first thought is to require gradients in the correct representation to be passed to change_gradient, so switching representation would happen first. Maybe it's fine to do it the other way too but it's not obvious to me.

That is also what I have in mind, that function should only change the metric and not representation

What remains is to decide for proper names (one could call the second also change_representer or something) and decide where to put the type-conversion and the parallel transport to the identity. I am still for doing both in project.

It seems reasonable to put both type conversion and parallel transport in project. By the way, vector projections of Rotations manifold doesn't do parallel transport currently as far as I can tell, it's a different vector transport.

I will check rotation then, but great that we agree, that both conversion and representation shift (to Lie algebra) should happen in the projection. I will look at that and adopt docs/function implementations.

[kellertuer]

I think, since change_gradient is a little more general, maybe even change_Riesz_representer would be a good name?

[kellertuer]

After I fixed the last few test coverage lines, I will start a small tutorial over on Manopt (maybe just Rayleigh again) on how to use a little bit of AD in Manopt.

[kellertuer]

While “just shortly adding” a test for the fallback to use the local metric, I am somehow hitting a wall on an ambiguity, where I thin transparency does something and then it does not find the local metric function I implemented. So I will have do take a looooong look into that, where local metric is maybe too transparent or something.

For now I do not yet understand why the exact function I implemented (which I thought would be called for the manifold&metric in the test) is not even mentioned in the ambiguous cases...

[mateuszbaran]

Yeah, we definitely don't have local_metric transparency tested well enough.

[kellertuer]

Hm, I am a little lost on local_metric. I never seems to call the function I think it should call and it behaves differently in debugger then when running the current last block of the metric.jl tests manually (including the types and function definitions from before).

So I do not know how it was supposed to work nor how it is currently actually non-working, since I can not debug the current error (when the debugger throws an error even earlier).

Any help appreciated. I think local_metric currently acts transparently even on MetricManifolds which it should not, since without the MetricManifold (on a non-default metric) this function does not make sense.

Edit 5: I removed the previous edits, since I do not understand why they went wrong, but I do not know why it is currently working, either.

[kellertuer]

After about 2 hours fiddling with a lot of transparency, maybe local_metric is fixed. But now get_vector and get_coordinates run into a tremendous amount of ambiguities, too.

I think the decorator pattern and bases being optional is something we should maybe redesign / avoid, since I feel like I can also spent a few weeks without sleep just trying to fix all this stuff and it just breaks even the last braincell after 2 hours.

[mateuszbaran]

I think I'd vote for just not using the decorator pattern for metric-related functions. There is just not enough benefit to justify the effort needed to make it work. get_vector and get_coordinates require a lot of disambiguation but work decently enough now. They should likely be simplified though but I don't think it's an urgent issue.

[kellertuer]

I might have found the way to get the tests running. Note that in the current test manifold I had to

function Manifolds.get_vector!(
    M::MetricManifold{ℝ,<:TestEuclidean,<:TestEuclideanMetric},
    X,
    ::Any,
    c,
    ::DefaultOrthogonalBasis{ℝ,<:ManifoldsBase.TangentSpaceType},
)

do quite a detailed typing on the basis to resolve the ambiguity.

[kellertuer]

It seems that sometimes Format & Documenter Fail because some packages could not be loaded. I just restarted both, since they should work. I also finished code coverage, so this should be good to go.

[mateuszbaran]

Great, I'll take a look at it again soon.

[mateuszbaran]

I think with some documentation polishing this should be ready to go 🙂

[mateuszbaran]

I think we can merge this when CI passes and finally resolve #17 🎉

[kellertuer]

Was a way to go, but I am happy we have that now :)