Skip to content

Use static arrays for eigen-calculations#182

Merged
fredrikekre merged 9 commits intoFerrite-FEM:masterfrom
KnutAM:kam/staticeigen
Apr 24, 2022
Merged

Use static arrays for eigen-calculations#182
fredrikekre merged 9 commits intoFerrite-FEM:masterfrom
KnutAM:kam/staticeigen

Conversation

@KnutAM
Copy link
Copy Markdown
Member

@KnutAM KnutAM commented Apr 21, 2022

Won't directly solve any issues, but rather move the responsibility of maintaining eigenvalue calculations to StaticArrays.jl
#173 and potentially #174/#175 (provided ForwardDiff 575) could be solved by this.
Gives less code base to maintain
Benchmarks to come soon...

@KnutAM
Copy link
Copy Markdown
Member Author

KnutAM commented Apr 22, 2022

Benchmark (after b4f893a)

using StableRNGs, BenchmarkTools, Random, Tensors
Base.rand(rng::AbstractRNG, ::Type{TT}) where{TT<:SymmetricTensor} = TT(ntuple((_)->rand(rng), Tensors.n_components(TT)))
rng=StableRNG(1);
for dim=1:3
        a=rand(rng,SymmetricTensor{2,dim})
        println("dim=$dim");
        @btime eigen($a);
        @btime eigvals($a);
        @btime eigvecs($a);
end
dim eigen eigvals eigvecs version
1 0.800 0.900 0.800 master
1 1.000 0.800 1.000 PR
2 6.700 5.800 6.000 master
2 6.700 1.200 5.800 PR
3 488.205 488.205 488.718 master
3 513.542 43.088 513.021 PR

So there is a 5-6 % performance hit for 3-dimensional eigenvector calculation.
But the advantage with the PR's approach is that StaticArrays' eigvals calculation could be used by adding a single line of code, improving the performance for calculating only eigenvalues

Note on 4th order
There is also a potential performance benefit of using StaticArrays for 4th order, 2-dimensional tensors (although this shouldn't be so common to do in a performance critical code?) Excluding the conversion, StaticArrays is a lot faster and without allocations.
481.538 ns (0 allocations: 0 bytes) compared to 7.450 μs (86 allocations: 7.92 KiB)
But the conversion is more tricky (not necessarily very costly), requiring e.g. a tomandel for creating static arrays.
This conversion might be useful in other cases too, but I think it makes sense to consider a separate PR...

@KristofferC
Copy link
Copy Markdown
Collaborator

Seems like an OK performance hit to take.

@KnutAM KnutAM marked this pull request as ready for review April 22, 2022 12:37
Comment thread Project.toml
Comment thread src/eigen.jl Outdated
Comment thread src/eigen.jl Outdated
Comment thread src/eigen.jl Outdated
Comment thread src/eigen.jl Outdated
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 22, 2022

Codecov Report

Merging #182 (ff01fec) into master (2e9ecf2) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   97.91%   97.84%   -0.07%     
==========================================
  Files          16       16              
  Lines        1341     1206     -135     
==========================================
- Hits         1313     1180     -133     
+ Misses         28       26       -2     
Impacted Files Coverage Δ
src/Tensors.jl 81.81% <ø> (ø)
src/math_ops.jl 100.00% <ø> (ø)
src/eigen.jl 100.00% <100.00%> (+1.40%) ⬆️

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 2e9ecf2...ff01fec. Read the comment docs.

@KnutAM
Copy link
Copy Markdown
Member Author

KnutAM commented Apr 22, 2022

I can remove to_smatrix(::Tensor) (not used) and add test for eigvecs before it is ready for merge!
I'll look into that tomorrow!

@KnutAM
Copy link
Copy Markdown
Member Author

KnutAM commented Apr 23, 2022

Ready from my point of view now!
Let me know if I should change something more!

@fredrikekre fredrikekre merged commit 8ae464b into Ferrite-FEM:master Apr 24, 2022
@KnutAM KnutAM deleted the kam/staticeigen branch June 14, 2022 20:57
@KnutAM KnutAM mentioned this pull request Jun 14, 2022
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.

4 participants