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

Stable hyperbolic functions #631

Closed hajg-ijk closed 1 year ago

hajg-ijk commented 1 year ago

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

codecov[bot] commented 1 year ago

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%> (ø)

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

mateuszbaran commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kellertuer commented 1 year ago

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 commented 1 year ago

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

hajg-ijk commented 1 year ago

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

mateuszbaran commented 1 year ago

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.