Make the current ODE-based exp solver only work in charts

[mateuszbaran]

That's a quick sketch of the change discussed in #429 . This ODE-based implementation of exp only works on R^n with a metric (or a connection) so this makes it explicit. The scaled sphere test no longer works because it can't with this solver. TrivialEuclideanAtlas is introduced for cases where we just want R^n with a metric.

This isn't particularly well thought-through but may be a good direction.

[kellertuer]

This looks nice, but I thought it should become only a special case, where we use this approach in solve_exp_ode and not the new default (since we introduce the generic default actually in #429)?

[mateuszbaran]

Yes, sure, that's just a special case but I don't want to introduce the generic one in this PR 🙂 . This is just a small improvement to the current code, and #429 can easily turn it into a special case.

I decided to make this a separate PR because it's essentially non-breaking and introduces a small new feature.

[kellertuer]

but it introduces quite a merge conflict when trying to get it over to the other PR now.

[mateuszbaran]

Locally I got a completely clean merge.

[mateuszbaran]

Do you maybe have some changes not pushed to github?

[kellertuer]

Eh, I did not try to merge, sorry, then I was mistaken, but how did that work, we both edited the solve-ode function? Hm, maybe I have something wrongly in mind – then never mind.

[mateuszbaran]

No problem. You've only edited a single line of that file: https://github.com/JuliaManifolds/Manifolds.jl/pull/429/files#diff-d55d52705ea227d3a35e496dcfae2d0d7beaa6174912d8435498f49967c970bbL8-R8 that I did not change.

[kellertuer]

Ok.

Another question, so with this you delete all variants that would have worked with nice other bases (ONB for example), right? Is that intentional or should we maybe keep that?

[mateuszbaran]

This is intentional because other bases don't guarantee correct results with the current code, at least as far as I understand it. Bases introduce vector transports through using the same coefficients at different points, and we don't require that this agrees with the connection of the manifold.

[kellertuer]

with the old variant, an ONB (orthogonal also wroks and I think even default should work) and an arbitrary retraction (that is not exp) should work in the setting of the other package.

Do you plan here to follow the path of replacing the default exp!since it is only an approximation?

[mateuszbaran]

with the old variant, an ONB (orthogonal also wroks and I think even default should work) and an arbitrary retraction (that is not exp) should work in the setting of the other package.

I don't really see how. In this call: christoffel_symbols_second(M, x, B; backend=backend) there is no connection between Christoffel symbols at different points in the general case.

[mateuszbaran]

Do you plan here to follow the path of replacing the default exp!since it is only an approximation?

That's not breaking so I could do it here.

[mateuszbaran]

with the old variant, an ONB (orthogonal also wroks and I think even default should work) and an arbitrary retraction (that is not exp) should work in the setting of the other package.

I don't really see how. In this call: christoffel_symbols_second(M, x, B; backend=backend) there is no connection between Christoffel symbols at different points in the general case.

Could you maybe link some text where geodesics are derived numerically but not in a single chart (or a series of charts)?

[kellertuer]

with the old variant, an ONB (orthogonal also wroks and I think even default should work) and an arbitrary retraction (that is not exp) should work in the setting of the other package.

I don't really see how. In this call: christoffel_symbols_second(M, x, B; backend=backend) there is no connection between Christoffel symbols at different points in the general case.

Could you maybe link some text where geodesics are derived numerically but not in a single chart (or a series of charts)?

Eh? It might be late, but I never talked about using or not using charts. But given an ONB (see the old implementation and get_vector) you can work in an implicit local chart using a retraction and an ONB. But the ONB can also just be a basis to form that implicit local chart in a tangent space (using the inverse of a retraction) – or think the other way around (parametrisation instead of charts): given c in R^d -> use Basis to construct tangent vector, use retraction to parametrise point. Nowhere in that do I need the ONB property.

[mateuszbaran]

I think I still don't follow... using low-order retractions in solve_exp_ode is one way to improve it but it's not the goal of this PR.

[kellertuer]

My only point is, the default solve_exp_ode did something that was valid and worked – your more specialised version is – for sure – cool and nice for your new retraction and the basis – but deleting the previous (more general) approach is not what that should do.

So could we please keep the old version as well? It works for far more bases (and with a proper fix on the other PR also works with the new Improved differentiation backends).

My only point is really that your method should not replace the current one but be a specialised version of it (via dispatch).

[mateuszbaran]

Hm, OK, we can keep the old variant but I'd like to document its requirements regarding the basis then -- they are not obvious to me at least.

[kellertuer]

Sure, I will try to find time for the new type and document that approach thoroughly.