Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce embedded vector transport #178

Merged
merged 11 commits into from
Dec 15, 2023

Conversation

kellertuer
Copy link
Member

as proposed in JuliaManifolds/Manifolds.jl#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.

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mateuszbaran mateuszbaran added the preview docs Add this label if you want to see a PR-preview of the documentation label Dec 13, 2023
@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Dec 13, 2023
@kellertuer
Copy link
Member Author

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
Copy link
Member Author

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.

NEWS.md Outdated Show resolved Hide resolved
src/vector_transport.jl Outdated Show resolved Hide resolved
src/vector_transport.jl Outdated Show resolved Hide resolved
src/vector_transport.jl Outdated Show resolved Hide resolved
src/vector_transport.jl Outdated Show resolved Hide resolved
src/vector_transport.jl Outdated Show resolved Hide resolved
Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if all tests in Manifolds.jl pass? If yes, that this LGTM.

@kellertuer
Copy link
Member Author

Ah that is maybe a good idea to do ;)

@kellertuer
Copy link
Member Author

kellertuer commented Dec 14, 2023

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
Copy link
Member Author

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.

@kellertuer kellertuer merged commit f5d3189 into master Dec 15, 2023
15 checks passed
@kellertuer kellertuer deleted the kellertuer/embedded-vector-transport branch May 4, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants