Closed mateuszbaran closed 10 months ago
Merging #677 (3d14c02) into master (848f618) will increase coverage by
0.00%
. The diff coverage is100.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% <ø> (ø) |
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!
Ah, maybe check for code coverage as well (but it seems to still be running?)
Coverage is fine now, the only issue is the randomly failing integration test but there I just hope it will resolve itself.
I will also take a look at other complex Lie groups before merging this PR.
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.
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.
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.
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)
vee
andhat
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?
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.
I would even consider that a bug fix.
Cool, I will make another PR then.
This should fix #668 . Now we have: