Some issues with allocation on complex manifolds

[mateuszbaran]

This should fix #668 . Now we have:

julia> G = Unitary(2)
Unitary(2)

julia> eltype(allocate_result(G, typeof(rand), identity_element(G)))
ComplexF64 (alias for Complex{Float64})

julia> allocate_result(G, typeof(rand))
2×2 Matrix{ComplexF64}:
 6.91456e-310+6.91456e-310im  6.91463e-310+6.91456e-310im
 6.91456e-310+6.91456e-310im  6.91456e-310+6.91457e-310im

[codecov]

Codecov Report

Merging #677 (3d14c02) into master (848f618) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #677   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files         108      108           
  Lines       10675    10678    +3     
=======================================
+ Hits        10630    10633    +3     
  Misses         45       45           

Files	Coverage Δ
src/Manifolds.jl	86.66% <ø> (ø)
src/groups/general_unitary_groups.jl	100.00% <100.00%> (ø)
src/groups/group.jl	100.00% <100.00%> (ø)
src/groups/unitary.jl	100.00% <ø> (ø)
src/manifolds/GeneralUnitaryMatrices.jl	100.00% <ø> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[kellertuer]

Ah, maybe check for code coverage as well (but it seems to still be running?)

[mateuszbaran]

Coverage is fine now, the only issue is the randomly failing integration test but there I just hope it will resolve itself.

[mateuszbaran]

I will also take a look at other complex Lie groups before merging this PR.

[Affie]

I will also take a look at other complex Lie groups before merging this PR.

While you are in this area, I've had issues with the unit quaternion group as well. It may have been finger trouble, but couldn’t get a lot of the functionality to work. For example get_coordinates and get_vector. We are using SO3 so not a blocker.

[mateuszbaran]

What exactly doesn't work with unit quaternions? get_vector and get_coordinates might be a bit non-intuitive there because you need to indicate that you want real coefficients like here:

julia> using Manifolds

julia> using Quaternions

julia> p = QuaternionF64(
           0.4815296357756736,
           0.6041613272484806,
           -0.2322369798903669,
           0.5909181717450419,
       )
QuaternionF64(0.4815296357756736, 0.6041613272484806, -0.2322369798903669, 0.5909181717450419)

julia> get_coordinates(G, p, QuaternionF64(0, 1, 2, 3), DefaultOrthonormalBasis(ℍ))
3-element StaticArraysCore.SVector{3, Float64} with indices SOneTo(3):
 1.0
 2.0
 3.0

Note DefaultOrthonormalBasis(ℍ) instead of DefaultOrthonormalBasis(). I'm not sure this convention was the right decision but it doesn't seem worth a rework.

[mateuszbaran]

I've taken a look at other groups and they don't seem to have similar issues. I've just made some cleanup while reading the code and exported number_of_coordinates. ManifoldsBase.jl already exports that function so I was surprised it's not exported here.

[Affie]

Thanks, so it was my mistake then. I missed the DefaultOrthonormalBasis(ℍ) . The error is related to allocations so thought it was related, vee and hat might be broken though.
ERROR: MethodError: no method matching similar(::QuaternionF64, ::Type{QuaternionF64}, ::Int64)

[kellertuer]

vee and hat might be broken though. ERROR: MethodError: no method matching similar(::QuaternionF64, ::Type{QuaternionF64}, ::Int64)

Are you maybe accidentially entering this with integer numbers instead of floats?

[mateuszbaran]

The problem with vee and hat is that they perhaps should pick the real-coefficient, basis. @kellertuer would you be fine with changing

vee(M::AbstractManifold, p, X) = get_coordinates(M, p, X, VeeOrthogonalBasis())

to

vee(M::AbstractManifold, p, X) = get_coordinates(M, p, X, VeeOrthogonalBasis(number_system(M))

?
It's technically breaking but makes them more useful IMO.

[kellertuer]

I would even consider that a bug fix.

[mateuszbaran]

Cool, I will make another PR then.