Package maintenance

[dkarrasch]

Sorry @andreasnoack, you merged too quickly. 😄

I found that the branching version is significantly faster than the one using mod. For instance, Matrix(C) for a 100x100 circulant matrix takes 82µs with mod vs. 15µs with the branching. The single indexing method should be obsolete since this is handled by Base. If we want to leave it anyway, then we should perhaps compute a single j, i = divrem(i - 1, size(A, 1)); return A[i+1, j+1].

Closes #23. And closes #30.

[coveralls]

Coverage increased (+14.4%) to 91.453% when pulling d2a223b on dkarrasch:master into 7ed5934 on JuliaMatrices:master.

[codecov]

Codecov Report

Merging #67 (d2a223b) into master (7ed5934) will increase coverage by 14.41%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master      #67       +/-   ##
===========================================
+ Coverage   77.03%   91.45%   +14.41%     
===========================================
  Files           2        2               
  Lines         479      468       -11     
===========================================
+ Hits          369      428       +59     
+ Misses        110       40       -70     

Impacted Files	Coverage Δ
src/ToeplitzMatrices.jl	91.55% <100.00%> (+17.71%)	⬆️

---

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 7ed5934...d2a223b. Read the comment docs.

[dkarrasch]

It also speeds up small circulant matrix-vector multiplication:

juli> using ToeplitzMatrices, BenchmarkTools

julia> C = Circulant(rand(250));

julia> x = rand(250);

julia> @btime $C*$x;
  57.374 μs (1 allocation: 2.06 KiB) # PR
  451.078 μs (1 allocation: 2.06 KiB) # current

[dkarrasch]

There's some more to come, so please hold the line...

[dkarrasch]

I ran Aqua.jl and fixed a couple of ambiguity issues. Among these were the wrong usage of Adjoint types, so I fixed multiplication of Circulants and their adjoints, and added some tests. While I was at it, I also added mul! methods for Hankel, which yields a significant speed-up in terms of performance, at the expense of more allocations:

julia> using Arpack, ToeplitzMatrices, Random

julia> Random.seed!(1234)

julia> H = Hankel(rand(250, 250));

julia> using BenchmarkTools

julia> @btime eigs($H);
  16.261 ms (766 allocations: 91.70 KiB) # PR
  32.388 ms (778 allocations: 92.14 KiB) # latest release

julia> Random.seed!(1234)

julia> H = Hankel(rand(500, 500));

julia> using FFTW

julia> @btime eigs($H);
  66.520 ms (9217 allocations: 6.34 MiB) # PR
  133.230 ms (791 allocations: 151.12 KiB) # latest release

Perhaps the view with the negative stride should be materialized? I'll experiment with it some further.

[dkarrasch]

Ok, it feels like (almost) every untested function was in a broken state. 🤣 By now, only a couple of dimension mismatch lines are uncovered. So this is ready for review and merging.

[dlfivefifty]

This package was sorely in need of some love.. thanks so much!

[dkarrasch]

Now, I believe I'm really done with the maintenance. I've included Aqua-testing for code quality, but turned off recursive ambiguity testing (otherwise it would fail for ambiguities in which this package is not involved directly). A few details to consider before merging: I have turned Hankel into an immutable type. After all, it is just a wrapper of something, so construction should cheap. Also, at least within the package there is no usage of mutability. One can still change entries in the defining vectors, of course. This change is technically breaking. Moreover, I have restricted the second factor in Hankel-vecormat-multiplication to strided ones. Technically, multiplication with abstract vecormats was possible, but fell back to generic linalg methods from LinearAlgebra, because mul! of Toeplitz matrices is restricted to strided arrays as well. That multiplication still works, but when asking for the used method via @which one gets directed to LinearAlgebra directly, instead of making it look like a method from ToeplitzMatrices.jl is called. Otherwise, this PR includes only fixes of syntax errors and adds tests.

Perhaps this should bump the minor version to v0.8 then?

[dkarrasch]

Gentle bump. I guess having excellent coverage will be good for the other open PRs.

[andreasnoack]

Thanks for the maintenance here