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

Fix random point / vector allocations #653

Merged
merged 6 commits into from
Sep 24, 2023
Merged

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Sep 22, 2023

This is the companion PR to JuliaManifolds/ManifoldsBase.jl#168 to fix using TangentSpaceAtPoint on non-matrix-type manifolds.

Resolves #652

@kellertuer kellertuer marked this pull request as ready for review September 22, 2023 12:51
@codecov
Copy link

codecov bot commented Sep 22, 2023

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%) ⬇️
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

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

@kellertuer
Copy link
Member Author

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 kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Sep 23, 2023
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.

LGTM. Maybe up to the point that we still might want to have in-place rand! for PositiveNumbers?

@kellertuer
Copy link
Member Author

kellertuer commented Sep 23, 2023

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

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

@kellertuer kellertuer merged commit 9337b86 into master Sep 24, 2023
16 of 30 checks passed
@kellertuer kellertuer deleted the kellertuer/fix-rand-FR branch May 4, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Allocation for TagentSpaceAtPoint fails
2 participants