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
366 stars 52 forks source link

Implement the complex functions (allocation still not working) #699

Closed kellertuer closed 7 months ago

kellertuer commented 7 months ago

This should (up to specifying the right basis) resolve https://github.com/JuliaManifolds/Manopt.jl/issues/340

For now there is a few things to check though

Here is a bit of test code that I try to verify the new method with

a) real check for comparison

M1 = Sphere(2)
p1 = rand(M1)
X1 = rand(M1; vector_at=p1) 
c1 = get_coordinates(M1, p1, X1, DefaultOrthonormalBasis())
# Since this is an ONB, the norm of X1 in the TpM and c1 (usual norm) should be equal
norm(M1, p1, X1) ≈ norm(c1)

The same should hold now if we perform that on M2 = Sphere(2, ℂ)

M2 = Sphere(2, ℂ)
p2 = rand(M2)
X2 = rand(M2; vector_at=p2) 
c2= get_coordinates(M2, p2, X2, DefaultOrthonormalBasis()) #probably with `\bbC`?
# Since this is an ONB, the norm of X2 in the TpM and c2 (usual norm) should be equal (still TODO)
norm(M2, p2, X2) ≈ norm(c2)
codecov[bot] commented 7 months ago

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (81a43d4) 99.57% compared to head (618ffe4) 99.33%.

:exclamation: Current head 618ffe4 differs from pull request most recent head 31f4707. Consider uploading reports for the commit 31f4707 to get more accurate results

Files Patch % Lines
src/manifolds/Sphere.jl 0.00% 26 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #699 +/- ## ========================================== - Coverage 99.57% 99.33% -0.25% ========================================== Files 108 108 Lines 10713 10739 +26 ========================================== Hits 10668 10668 - Misses 45 71 +26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mateuszbaran commented 7 months ago
  • complex coefficients in a real-valued basis, which is sometimes, especially for the sphere here a bit strange, since it would mean that in the North Pole the first component is only allowed to be purely imaniary for the spheres

I think we should restrict complex-coefficient bases to complex manifolds, while the complex sphere is actually a CR manifold but not a complex manifold.

mateuszbaran commented 7 months ago

Or, at least, I wouldn't add complex-coefficients bases to non-complex CR manifold without some understanding of the theory behind CR-manifolds.

kellertuer commented 7 months ago

That would again be something breaking and different to what we use those currently, but sure that is something to discuss.

kellertuer commented 7 months ago

I fixed the one allocation bug I had but now I notice that my idea might not work as intended (doing the same rotation as in the real case and taking the complex value of the first component as the last one). So I fear I have to close this since I have no clue how to compute an ONB or its coordinates here.