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

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

Closed
wants to merge 2 commits into from

Conversation

kellertuer
Copy link
Member

This should (up to specifying the right basis) resolve JuliaManifolds/Manopt.jl#340

For now there is a few things to check though

  • it seems the docs for the original get_coordinates (real-valued case) are not precise enough, the inner product for $\lambda$ seems wrong and the used $x$ is not defined. Will check. The same holds for the inverse get_vector
  • We should improve the docs in ManifoldsBase.jl and provide more details on the two approaches used for bases in the complex case
    • real coefficients in a complex-valued basis, which has the advantage that we have as many coefficients as the manifold dimension, and this should be the default
    • 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
    • currently it still seems that allocation is a bit wrong still, since the real coefficients should – for best of cases, also be real. On the sphere they also seem to be of wrong size still.

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)

Copy link

codecov bot commented Jan 1, 2024

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%.

❗ 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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mateuszbaran
Copy link
Member

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

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

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

@kellertuer
Copy link
Member Author

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.

@kellertuer kellertuer closed this Jan 2, 2024
@kellertuer kellertuer deleted the kellertuer/complex-sphere-coefficients branch May 4, 2024 17:39
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.

quasi_Newton has issue with negative memory size.
2 participants