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

Stable hyperbolic functions #631

Merged
merged 10 commits into from
Jun 20, 2023
Merged

Conversation

hajg-ijk
Copy link
Member

Replaces distance, exponential, and logarithmic maps to more numerically stable versions, as per the Matlab Manopt version.

@hajg-ijk hajg-ijk changed the title Hajg ijk/stable hyperbolic logs Stable hyperbolic functions Jun 16, 2023
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #631 (acc8e66) into master (836e716) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   99.01%   99.01%   -0.01%     
==========================================
  Files         104      104              
  Lines       10138    10137       -1     
==========================================
- Hits        10038    10037       -1     
  Misses        100      100              
Impacted Files Coverage Δ
src/manifolds/HyperbolicHyperboloid.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

Improving accuracy for Hyperbolic would be a good idea. Could you make a comparison of accuracy of the old and new versions? For example on Hyperbolic(2) and Hyperbolic(10) using some random points. You can compute a reference using BigFloat.

@kellertuer
Copy link
Member

We also noticed that parallel transport might be more stable in a different implementation.

Actually the ideas come from Manopt/Matlab, where they are documented as more stable – but then where would we put such an accuracy comparison? A small quarto where we define and compare both variants?

@kellertuer
Copy link
Member

kellertuer commented Jun 17, 2023

Thanks for your remarks Mateusz :)

I hacked a small start of a comparison; I can tell from that, that both implementations yield different results, but since I use the old one for the BigFloat, the old one has a smaller error than the new one. So I am not so sure what to compare to.

edit: But if I would see how to compare these, for one function, I can happily do the other functions as well. Especially also to look where the Manopt/Matlab code states that the current implementation we have might be more stable for larger tangent vectors (in norm), we could check that with a test as well.

@kellertuer
Copy link
Member

kellertuer commented Jun 18, 2023

edit: My original check had a bug, but the numbers are still in favour or the new distance (and hence exp/log).

kellertuer and others added 2 commits June 18, 2023 17:59
Co-authored-by: Mateusz Baran <mateuszbaran89@gmail.com>
@kellertuer
Copy link
Member

Hm, something broke on CI unrelated to this PR I think? See https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/5304573985/jobs/9600993369?pr=631

@mateuszbaran
Copy link
Member

OrdinaryDiffEq precompilation fails, that looks unrelated to Manifolds.jl.

@hajg-ijk
Copy link
Member Author

Nightly checks are failing. Is it still okay to squash and merge or not?

@mateuszbaran
Copy link
Member

Yes, it's perfectly fine to squash and merge with nightly failures. I like to checked them once in a few months to see if there is something to report but that's all.

@hajg-ijk hajg-ijk merged commit dae9e52 into master Jun 20, 2023
27 of 30 checks passed
@kellertuer kellertuer deleted the hajg-ijk/stable-hyperbolic-logs 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.

None yet

3 participants