JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
368 stars 53 forks source link

Fix random point / vector allocations #653

Closed kellertuer closed 11 months ago

kellertuer commented 11 months ago

This is the companion PR to https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/168 to fix using TangentSpaceAtPoint on non-matrix-type manifolds.

Resolves #652

codecov[bot] commented 11 months ago

Codecov Report

Merging #653 (da47217) into master (b5cdb3c) will decrease coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   99.22%   99.18%   -0.04%     
==========================================
  Files         106      106              
  Lines       10433    10440       +7     
==========================================
+ Hits        10352    10355       +3     
- Misses         81       85       +4     
Files Changed Coverage Δ
src/manifolds/FixedRankMatrices.jl 98.20% <100.00%> (-0.03%) :arrow_down:
src/manifolds/PositiveNumbers.jl 100.00% <100.00%> (ø)
src/manifolds/SymmetricPositiveDefinite.jl 100.00% <100.00%> (ø)
src/manifolds/VectorBundle.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kellertuer commented 11 months ago

For positive numbers the problem was, that the allocating version has to return a number and can not allocate anything, for SPDPoint we needed an allocation of a tangent vector – but then now rand should be overall more stable when it comes especially to random tangents.

kellertuer commented 11 months ago

Sure, then we just have to check that it is also covered by tests, the new (allocating) one basically covers all cases we currently have in the tests.

edit: Had an idea, stole the zero-dimensional arrays from the circle and added both rand! and a test.

kellertuer commented 11 months ago

The new four uncovered lines are false positives (function headers but the functions are green