[WIP] Testsuite

[lkdvos]

This is an attempt to tackle #91 .

The main goal is to provide some way to reduce the amount of code duplication in our tests here, but additionally to think about how this might affect downstream adopters of this package.

I've set up the QR tests to give an idea of what I have in mind, feedback greatly appreciated!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/implementations/qr.jl	43.54% <100.00%> (-53.08%)	⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jutho]

With the LAPACK methods, there is some interplay between pivoted and blocksize, but I guess the new tests do not test them simultaneously so everything is fine.

[lkdvos]

I was considering adding some dedicated LAPACK methods to test these very specific behaviors, so have a large testsuite to do basic testing, but then add additional tests where necessary.

[kshyatt]

I think this should be relatively easy to make work with the GPU code, but we might want to break up some of the files into multiple functions that implement subsets of the tests to avoid scalar indexing/unsupported operations in some cases.

[github-actions]

Your PR requires formatting changes to meet the project's style guidelines.
Please consider running Runic (git runic main) to apply these changes.

Click here to view the suggested changes.

diff --git a/test/testsuite/polar.jl b/test/testsuite/polar.jl
index af747aa..83c5aa7 100644
--- a/test/testsuite/polar.jl
+++ b/test/testsuite/polar.jl
@@ -58,7 +58,7 @@ function test_right_polar(
             Ac = deepcopy(A)
             P, Wᴴ = right_polar(A; alg)
             @test eltype(Wᴴ) == eltype(A) && size(Wᴴ) == (size(A, 1), size(A, 2))
-            @test eltype(P)  == eltype(A) && size(P)  == (size(A, 1), size(A, 1))
+            @test eltype(P) == eltype(A) && size(P) == (size(A, 1), size(A, 1))
             @test P * Wᴴ ≈ A
             @test isisometric(Wᴴ; side = :right)
             @test isposdef(P)
diff --git a/test/testsuite/projections.jl b/test/testsuite/projections.jl
index 02cef21..2f51533 100644
--- a/test/testsuite/projections.jl
+++ b/test/testsuite/projections.jl
@@ -86,7 +86,7 @@ function test_project_hermitian(
         end
 
         # test approximate error calculation
-        A  = normalize!(randn(rng, T, m, m))
+        A = normalize!(randn(rng, T, m, m))
         Ah = project_hermitian(A)
         Aa = project_antihermitian(A)
 
@@ -111,12 +111,12 @@ function test_project_isometric(
     return @testset "project_isometric! $summary_str" begin
         algs = (PolarViaSVD(), PolarNewton())
         @testset "algorithm $alg" for alg in algs
-            A    = instantiate_matrix(T, sz)
-            Ac   = deepcopy(A)
-            k    = min(size(A)...)
-            W    = project_isometric(A, alg)
+            A = instantiate_matrix(T, sz)
+            Ac = deepcopy(A)
+            k = min(size(A)...)
+            W = project_isometric(A, alg)
             @test isisometric(W)
-            W2   = project_isometric(W, alg)
+            W2 = project_isometric(W, alg)
             @test W2 ≈ W # stability of the projection
             @test W * (W' * A) ≈ A
 

[Jutho]

Suggested change

	T = eltype(A)
	return if T <: Real
	all(≥(zero(T)), diagview(A))
	else
	all(≥(zero(real(T))), real(diagview(A))) &&
	all(≈(zero(real(T))), imag(diagview(A)))
	end
	all(x -> x ≈ abs(x), diagview(A))

[lkdvos]

We can do this, but it does change the tests, -eps() would have this property but it wouldn't have been properly "positive". sign(x) == 1 might work but I'm not sure how accurate that is

[Jutho]

julia> eps() ≈ -eps()
false

[lkdvos]

Fair enough, but how about:

julia> 1.0 + 1e-12im ≈ abs(1.0 + 1e-12im)
true

😉

Jokes aside, I think our current implementation is really such that the imaginary part is hard zero and the real part is hard positive, I don't have a strong opinion about whether or not this is a requirement either (i.e. are we assuming that we can always make this exactly true for weird Number types, or is it sometimes only approximately positive?), so I'll just change this :)

[Jutho]

Well indeed, I also think we currently require strict positivity, but I was confused by all(≈(zero(real(T))), imag(diagview(A))). Is this equivalent to all(iszero, imag(diagview(A))). If you want the latter, then clearly we want x == abs(x).

[Jutho]

or simply all(isposdef, diagview(A)) (but this requires LinearAlgebra.isposdef)