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

Add check_point for complex SymplecticStiefel #735

Closed
mateuszbaran opened this issue Aug 8, 2024 · 13 comments · Fixed by #732
Closed

Add check_point for complex SymplecticStiefel #735

mateuszbaran opened this issue Aug 8, 2024 · 13 comments · Fixed by #732
Labels
extend manifold This issue proposes/asks for new functions to extend an existing manifold

Comments

@mateuszbaran
Copy link
Member

The existing one only works for real numbers.

@mateuszbaran mateuszbaran added good first issue Good for newcomers extend manifold This issue proposes/asks for new functions to extend an existing manifold labels Aug 8, 2024
@kellertuer
Copy link
Member

Can you maybe provide an example? I just did

julia> M = SymplecticStiefel(4,2,ℂ)
SymplecticStiefel(4, 2; field=ℂ)

julia> p = rand(M)
4×2 Matrix{ComplexF64}:
 -0.998498+0.0im   -1.6602+0.0im
 -0.609752+0.0im  -1.18466+0.0im
  -2.05232+0.0im  -4.49237+0.0im
  0.820544+0.0im    1.7227+0.0im

julia> is_point(M,p)
true

If I assume that rand us correct, the point check also works just fine?

@mateuszbaran
Copy link
Member Author

Does it actually return false for incorrect points? See here:

function check_point(M::SymplecticStiefel{<:Any,ℝ}, p; kwargs...)
. It most likely hits some default implementation that only checks the embedding.

I noticed it when checking this ambiguity:

check_point(M::SymplecticStiefel{<:Any, ℝ}, p; kwargs...) @ Manifolds ~/.julia/dev/Manifolds/src/manifolds/SymplecticStiefel.jl:113
check_point(M::SymplecticStiefel, p::StiefelPoint; kwargs...) @ Manifolds ~/.julia/dev/ManifoldsBase/src/point_vector_fallbacks.jl:119

@kellertuer
Copy link
Member

right, that I did not check, I thought it was failing somwhere. You are right then it probably just checks the embedding, i.e. field type and dimensions for now.

but p^+p just has to be checked on complex as well then. I can check whether I see how much to do that is (maybe even just removing R)

@kellertuer
Copy link
Member

kellertuer commented Aug 10, 2024

Hm, I checked a bit the code. First of all, the complex part (also of p above) is zero, so I am not sure we can generate (non real) symplectic matrices. So I am also not sure what to actually additionally check.

To conclude, I would consider this not fully installed, but also not much I can do here (neither good-first nor easy to extend) since I am not aware of Literature that could help us here.

edit: One could make the corresponding doc strings a bit more precise, that for those we do not know yet how that has to work in the complex case. That improbably also why it was not implemented in the first place.

@kellertuer
Copy link
Member

It seems canonical_project is wrong as well for complex ones – but I think again we should mainly further restrict those then, because I am not much aware of complex symplectic Stiefel literature.

@mateuszbaran mateuszbaran removed the good first issue Good for newcomers label Aug 10, 2024
@mateuszbaran
Copy link
Member Author

I see, I didn't really check how difficult that would be to add because it looked quite simple but maybe it isn't actually.

@kellertuer
Copy link
Member

I think the problem is that the main idea of the model here is to cover real-valued things; maybe I was too optimistic adding the complex case to the manifold at all. For example the paper is also only called the real-valued symplectic Stiefel.
So it might be better to remove the complex case here.

@kellertuer
Copy link
Member

Baby the best is then to remove the complex case (we have a breaking PR open anyways) also for symplectic Grassmann?

I could not find any literature on the complex case

@mateuszbaran
Copy link
Member Author

I think it would be enough to limit signatures of methods that don't work in the complex case.

@kellertuer
Copy link
Member

But that is the current case already. Then the complex case falls back to our optimistic check-default that returns nothing.

@mateuszbaran
Copy link
Member Author

Ah, right, then maybe let's indeed limit symplectic Stiefel and Grassmann to real numbers for now.

@kellertuer
Copy link
Member

Great. On the 0.10 PR? Could you add that or shall I?

@mateuszbaran
Copy link
Member Author

I will do that on the 0.10 PR.

@mateuszbaran mateuszbaran linked a pull request Aug 16, 2024 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extend manifold This issue proposes/asks for new functions to extend an existing manifold
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants