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 check_point for SPD #593

Merged
merged 2 commits into from
Apr 28, 2023
Merged

Fix check_point for SPD #593

merged 2 commits into from
Apr 28, 2023

Conversation

mateuszbaran
Copy link
Member

A small bugfix. eigvals returns complex eigenvalues in the provided test case, which are not comparable to 0. This, also, should be faster.

@kellertuer
Copy link
Member

LGTM,
could you also remove those two lines since we are at fixing small things :)

is_point(A.manifold, p, true)
is_point(A.manifold, q, true)

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #593 (d6ebea0) into master (8ccd5d8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   98.90%   98.90%   -0.01%     
==========================================
  Files          98       98              
  Lines        9897     9896       -1     
==========================================
- Hits         9789     9788       -1     
  Misses        108      108              
Impacted Files Coverage Δ
src/groups/rotation_action.jl 100.00% <100.00%> (ø)
src/manifolds/SymmetricPositiveDefinite.jl 100.00% <100.00%> (ø)
...tricPositiveDefiniteGeneralizedBuresWasserstein.jl 100.00% <100.00%> (ø)

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

@mateuszbaran
Copy link
Member Author

Sure, I will remove them.

@kellertuer
Copy link
Member

The CI seems to indicate that you probably have to adapt tolerances?

@mateuszbaran
Copy link
Member Author

I increased accuracy instead, I hope that's also fine 🙂

@mateuszbaran mateuszbaran merged commit 73cfc7b into master Apr 28, 2023
Comment on lines +97 to +98
# symmetrizing for better accuracy
copyto!(q, (q .+ q') ./ 2)

Choose a reason for hiding this comment

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

How is this increasing accuracy? How do you know the error in q is "symmetric", and reduced by symmetrization? Also, I wonder if this is optimal with respect to memory consumption? Perhaps something like

temp = m * Y * p * Y * m
temp .= temp .+ p .+ X
copyto!(q, temp)
# or
# copyto!(q, Symmetric(temp))
# or 
# copyto!(q, (temp .+ temp') ./ 2) # which first allocates the result and then copies it to `q`
# or, starting from Julia v1.10
# hermitianpart!(q, temp)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's more accurate than the previous implementation in the sense that the result is numerically closer to the manifold. I should've made a better comment there.

Maybe taking either the upper or lower part instead of average would be a better idea, I didn't check that. I didn't care much about memory consumption here because previous lines are already quite expensive.

@kellertuer kellertuer deleted the mbaran/fix-check-point-spd branch May 4, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants