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 support for calculating eigenvalues and eigenvectors #170

Merged
merged 11 commits into from
Oct 26, 2021

Conversation

hyrodium
Copy link
Collaborator

Before this PR

julia> eigvals(RotXYZ(1,2,3))
ERROR: Only hermitian matrices are diagonalizable by *StaticArrays*. Non-Hermitian matrices should be converted to `Array` first.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] #eigvals#619
   @ ~/.julia/packages/StaticArrays/vxjOO/src/eigen.jl:12 [inlined]
 [3] eigvals(a::RotXYZ{Float64})
   @ StaticArrays ~/.julia/packages/StaticArrays/vxjOO/src/eigen.jl:7
 [4] top-level scope
   @ REPL[2]:1

julia> eigvecs(RotXYZ(1,2,3))
3×3 StaticArrays.SMatrix{3, 3, ComplexF64, 9} with indices SOneTo(3)×SOneTo(3):
  0.130632+0.39127im    0.130632-0.39127im   0.812211+0.0im
  0.689496-0.0im        0.689496+0.0im       -0.22179+0.0im
 0.0867797-0.588989im  0.0867797+0.588989im  0.539559+0.0im

After this PR

julia> eigvals(RotXYZ(1,2,3))
3-element StaticArrays.SVector{3, ComplexF64} with indices SOneTo(3):
 -0.7278678429175085 + 0.6857174368838911im
 -0.7278678429175085 - 0.6857174368838911im
                 1.0 + 0.0im

julia> eigvecs(RotXYZ(1,2,3))
3×3 StaticArrays.SMatrix{3, 3, ComplexF64, 9} with indices SOneTo(3)×SOneTo(3):
  -0.130632+0.39127im     0.39127-0.130632im   -0.812211+0.0im
  -0.689496+0.0im             0.0-0.689496im     0.22179+0.0im
 -0.0867797-0.588989im  -0.588989-0.0867797im  -0.539559+0.0im

@hyrodium hyrodium requested a review from andyferris October 23, 2021 15:00
@hyrodium hyrodium changed the title Add support for calucating eigenvalues and eigenvectors Add support for calculating eigenvalues and eigenvectors Oct 23, 2021
@codecov
Copy link

codecov bot commented Oct 23, 2021

Codecov Report

Merging #170 (143f9aa) into master (e9b567f) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   85.95%   86.15%   +0.20%     
==========================================
  Files          12       13       +1     
  Lines        1289     1315      +26     
==========================================
+ Hits         1108     1133      +25     
- Misses        181      182       +1     
Impacted Files Coverage Δ
src/eigen.jl 100.00% <100.00%> (ø)
src/unitquaternion.jl 95.62% <0.00%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9b567f...143f9aa. Read the comment docs.

@hyrodium
Copy link
Collaborator Author

Ah, I was forgetting to add methods for Rotation{2}.
I'll deal with it tommorow.

Copy link
Contributor

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool.

Out of curiousity, what do you think you might do with it?

Do we support log yet?

Comment on lines +17 to +18
λs = eigvals(R)
vs = eigvecs(R)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much work is repeated here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't quite understand the question.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, some of the calculation going into the eigenvalues is probably being recalculated in eigvecs. Not the way it is currently written but it usual that it falls out that way.

I doubt it has a massive performance impact here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I've updated code and avoid converting to AngleAxis twice.

Before performance update

julia> R = rand(RotMatrix{3})
3×3 RotMatrix3{Float64} with indices SOneTo(3)×SOneTo(3):
  0.196251   -0.948589   0.248325
  0.979358    0.202123  -0.00188397
 -0.0484051   0.243568   0.968675

julia> @benchmark eigen(R)
BenchmarkTools.Trial: 10000 samples with 434 evaluations.
 Range (min  max):  234.707 ns   2.399 μs  ┊ GC (min  max): 0.00%  89.78%
 Time  (median):     238.424 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   246.933 ns ± 73.440 ns  ┊ GC (mean ± σ):  1.26% ±  3.77%

  ▃▆█▆▅▃▁▁▁▁             ▁▁▁        ▁                          ▁
  ████████████▇█▇▇▆▆▇▆▆▅█████▆▆▅▇▇▇█████▇▆▆▅▆▆▇▇▇▇▅▅▅▄▅▄▄▃▄▄▃▄ █
  235 ns        Histogram: log(frequency) by time       315 ns <

 Memory estimate: 208 bytes, allocs estimate: 1.

After performance update

julia> R = rand(RotMatrix{3})
3×3 RotMatrix3{Float64} with indices SOneTo(3)×SOneTo(3):
 -0.81819   -0.492907   -0.295987
 -0.287214  -0.0955672   0.953087
 -0.49807    0.864818   -0.0633776

julia> @benchmark eigen(R)
BenchmarkTools.Trial: 10000 samples with 700 evaluations.
 Range (min  max):  179.454 ns  929.644 ns  ┊ GC (min  max): 0.00%  80.02%
 Time  (median):     183.104 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   187.316 ns ±  40.447 ns  ┊ GC (mean ± σ):  1.17% ±  4.40%

   ▃▇██▇▆▄▃▄▄▃▂▁                                                ▂
  ███████████████▆▆▅▆▄▁▃▁▃▁▁▁▁▃▁▁▁▃▁▁▁▁▁▁▃▁▁▃▃▄▄▅▃▄▆▇▇▇▇▇███▇█▇ █
  179 ns        Histogram: log(frequency) by time        232 ns <

 Memory estimate: 208 bytes, allocs estimate: 1.

@hyrodium
Copy link
Collaborator Author

hyrodium commented Oct 25, 2021

Out of curiousity, what do you think you might do with it?

I don't have applications of eigenvalues of rotations for now. Just for fun:smile:

Do we support log yet?

I thought the return value type should be Lie algebra and this should be done after solving #30. But, I realized that we just need to return SMatrix{3,3} for now.

@hyrodium
Copy link
Collaborator Author

I've added the methods for 2d rotations, and I think this is ready to merge.

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.

2 participants