local_jacobian dimension bug.

[kellertuer]

I am currently taking a look at our differential stuff – since we nearly got AD in the embedding working already

I stumbled upon our Jacobins and noticed, that they might be wrong. Let me explain why. I am looking at

https://github.com/JuliaManifolds/Manifolds.jl/blob/9fc772a00e256db7f0ad51dfe3f063849128942c/src/manifolds/MetricManifold.jl#L450-L472

and I think @sethaxen you wrote this? My problem here is I think the dimension. If p is on the sphere (d+1-dimensional unit vector), we should do a d-dimensional Jacobean, but we actually do a d+1 dimensional one? Usually the Jacobian is expressed in charts and I think this is missing here. But I think I also have a solution. We can build an implicit chart given a basis in the tangent space (and we have a basis here anyways. The idea is as follows

1. Instead of q we could look at a function X -> exp(M,p,X). this would give the same function locally, just in the tangent space.
2. we can now get to the right dimension by “building” X with respect to its coefficients in a basis, i.e. as c -> exp(M, p, get_vector(M, p, c, B) (where B is our basis). Then the overall function reads

   c -> local_metric(M, exp(M, p, get_vector(M, p, c, B), B))
   

   would have the right dimensions (manifold_dimension(M) instead of size(p,1) - why the 1 anyways?).

Finally: Since we have a NoneBackend already, do we really need the dependency on FiniteDifferences.jl or can we load that optionally as we do for the other differentiation packages?

When my approach is ok, I can also add it to the code, I am revising that part anyways currently (and it provides me with nice automatic gradients already!) - so I already assign this to myself.

(related ( maybe also solved in that route: #88, since that is also just a small step there).

[mateuszbaran]

I think reworking this is a good idea. We probably should replace exp with default retraction.

One more thing, it isn't guaranteed that any arbitrary basis B would be a basis at a point different than B (for example cached bases won't work). So this would have to be restricted to DefaultOrthonormalBasis, or even InducedBasis of a RetractionAtlas.

[kellertuer]

The exp replacement is a good idea.

For the basis – I feel a little unsure, you are right it does not work with all bases, but restricting it would mean you have to allow new bases explicitly later.