[WIP] Testsuite #93
[WIP] Testsuite #93
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
test/qr.jl
Outdated
| m = 54 | ||
| for T in BLASFloats, n in (37, m, 63) | ||
| TestSuite.seed_rng!(123) | ||
| TestSuite.test_qr(T, (m, n)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
kshyatt
force-pushed
the
testsuite
branch
3 times, most recently
from
29dd784 to
cf7839a
Compare
| 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 |
There was a problem hiding this comment.
| 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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
or simply all(isposdef, diagview(A)) (but this requires LinearAlgebra.isposdef)
|
Edit: |
kshyatt
force-pushed
the
testsuite
branch
2 times, most recently
from
6f21e71 to
b6fd59e
Compare
|
I think the only factorization left to cover is |
kshyatt
force-pushed
the
testsuite
branch
4 times, most recently
from
81e2b9d to
178e892
Compare
|
Note to self + others: We probably want to add an additional part to the tests (presumably this does not have to be part of the suite, unless we can fit it in nicely), which tests the algorithms that are not selected automatically.
|
|
OK, I think everything is in here except for testing the different algorithms. I'll think a bit about a graceful way to do that... |
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!