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

Manifolds.jl/src/manifolds/MetricManifold.jl

Lines 450 to 472 in 9fc772a

	@doc raw"""
	local_metric_jacobian(
	M::AbstractManifold,
	p,
	B::AbstractBasis;
	backend::AbstractDiffBackend = diff_backend(),
	)
	Get partial derivatives of the local metric of `M` at `p` in basis `B` with respect to the
	coordinates of `p`, ``\frac{∂}{∂ p^k} g_{ij} = g_{ij,k}``. The
	dimensions of the resulting multi-dimensional array are ordered ``(i,j,k)``.
	"""
	local_metric_jacobian(::AbstractManifold, ::Any, B::AbstractBasis)
	function local_metric_jacobian(
	M::AbstractManifold,
	p,
	B::AbstractBasis;
	backend::AbstractDiffBackend=diff_backend(),
	)
	n = size(p, 1)
	∂g = reshape(_jacobian(q -> local_metric(M, q, B), p, backend), n, n, n)
	return ∂g
	end

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.