MichielStock / Kronecker.jl

A general-purpose toolbox for efficient Kronecker-based algebra.
MIT License
86 stars 14 forks source link

Type-stable KroneckerPower #102

Closed jishnub closed 3 years ago

jishnub commented 3 years ago

This PR removes the exponent from the type-domain of a KroneckerPower, relying on runtime checks instead. This improves performance significantly, even though getmatrices loses type-stability. Fixes #101.

On master

julia> A = collect(reshape(1:16, 4, 4));

julia> K1 = kronecker(A, 3);

julia> @btime $K1[1,1]
  1.730 μs (10 allocations: 320 bytes)
1

This PR

julia> @btime $K1[1,1]
  97.187 ns (1 allocation: 32 bytes)
1
codecov-commenter commented 3 years ago

Codecov Report

Merging #102 (a1cf1cb) into master (08c40e2) will decrease coverage by 0.86%. The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   90.95%   90.09%   -0.87%     
==========================================
  Files          10       10              
  Lines         619      646      +27     
==========================================
+ Hits          563      582      +19     
- Misses         56       64       +8     
Impacted Files Coverage Δ
src/kroneckerpowers.jl 93.18% <85.71%> (+1.87%) :arrow_up:
src/indexedkroncker.jl 77.27% <0.00%> (-7.54%) :arrow_down:
src/base.jl 92.85% <0.00%> (-0.58%) :arrow_down:
src/multiply.jl 100.00% <0.00%> (ø)
src/kroneckergraphs.jl 100.00% <0.00%> (ø)
src/kroneckersum.jl 84.09% <0.00%> (+0.18%) :arrow_up:
src/vectrick.jl 94.69% <0.00%> (+0.20%) :arrow_up:

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 08c40e2...a1cf1cb. Read the comment docs.

MichielStock commented 3 years ago

Nice improvement. When testing this branch, I noticed products between KroneckerPower and KroneckerProduct do not (always) work. I fixed this in #103. OK if I close this PR and merge your changes via the other branch?

jishnub commented 3 years ago

Yes sure!