search

JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
83 stars 8 forks source link

Introduce embedded vector transport #178

Closed kellertuer closed 7 months ago

kellertuer commented 7 months ago

as proposed in https://github.com/JuliaManifolds/Manifolds.jl/issues/624 this can be done quite generic. This PR also changes the vector transport dispatch path a bit so that it is the same way as retractions and inverse retractions (allocate earlier) which does not require any test change, so it should be nonbreaking.

This PR still needs a bit of testing, but doc strings should be complete already.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ce3f90b) 99.96% compared to head (d4d3cae) 99.96%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #178 +/- ## ========================================== - Coverage 99.96% 99.96% -0.01% ========================================== Files 26 26 Lines 3091 3068 -23 ========================================== - Hits 3090 3067 -23 Misses 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kellertuer commented 7 months ago

Decided to stay slightly longer than I try to usually this week to finish testing here as well. I think it works quite fine – and vector transports are closer to retractions now also in allocations – before they were somewhere in between, whether the default would allocate and call the in-place one or not – now it allocates as early as retractions as well. And none of the tests broke, and we have less code \o/

kellertuer commented 7 months ago

Hm we seem to loose 9 lines of coverage somewhere which seems to be totally unrelated – will have to check that. But feature and test wise this is ready to review.

kellertuer commented 7 months ago

Ah that is maybe a good idea to do ;)

kellertuer commented 7 months ago

It seems to “nearly work” just that the allocating manifolds like the circle need a bit of a tweak. But I can fix that on my next PR in Manifolds as well then.

edit: and it is really just two tests of vector_transport_direction that does fall back twice: (a) from direction to _to and then back to parallel transport. I am not sure why such a complicated fallback was run on circle anyways, but I can fix that on the next PR indeed.

kellertuer commented 7 months ago

The fix is tone in the atol= PR, so this should be good to go. I will wait for the other PR though, it seems fine to put them together in one new version.