-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
# Conflicts: # NEWS.md
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/ |
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. |
… now claimed uncovered lines.
Co-authored-by: Mateusz Baran <[email protected]>
…iaManifolds/ManifoldsBase.jl into kellertuer/embedded-vector-transport
There was a problem hiding this 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.
Ah that is maybe a good idea to do ;) |
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 |
The fix is tone in the |
# Conflicts: # NEWS.md
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.