Adding the necessary files to benchmark AMD.jl #38
Conversation
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=======================================
Coverage 95.16% 95.16%
=======================================
Files 3 3
Lines 124 124
=======================================
Hits 118 118
Misses 6 6 Continue to review full report at Codecov.
|
| sh 'printenv' | ||
| sh ''' | ||
| git clean -fd | ||
| git reset --hard |
There was a problem hiding this comment.
This line isn't in LDLFactorizations.jl. Should it be added there?
There was a problem hiding this comment.
Yes. It definitely should be added in LDLFactorizations.jl as well!
There was a problem hiding this comment.
Could you submit a PR there please?
benchmark/benchmarks.jl
Outdated
| # push!(ratio_symrcm, nnz(ldlt_symrcm) / nnz_A) | ||
| # push!(ratio_classic, nnz(ldlt_classic) / nnz_A) | ||
|
|
||
| SUITE["no_ordering"][name] = @benchmarkable collect(1:size($A,1)) |
There was a problem hiding this comment.
What you want here is to perform the factorization with ordering 1:n and count the number of nonzero elements in the L factor, and then the ratio nnz(L) / nnz(tril(A, -1)).
There was a problem hiding this comment.
Do you mean nnz(tril(A)) instead of nnz(tril(A, -1))?
There was a problem hiding this comment.
Well, this line used to say nnz(tril(A)) but I think it should be nnz(tril(A, -1)) because we don't store the diagonal of the L factor. So we want to compare the number of nonzeros in L (which is strictly lower triangular) to the number of nonzeros in the strict lower triangle of A.
There was a problem hiding this comment.
A is the matrix that we want to factorize so I suppose that it's relevant to count the nnz elements on its diagonal when A is symmetric.
With the LDL' factorization, L have a unit diagonal so we don't count the ones but we also have the elements of D.
When I did the benchmarks, I used the ratio (nnz(tril(L, -1)) + nnz(D)) / nnz(tril(A)).
There was a problem hiding this comment.
D must be full and diag(A) must be full for the factorization to exist. So I think it's sufficient to compare nnz(L) to nnz(tril(A, -1)). The diagonal of L is not stored.
benchmark/benchmarks.jl
Outdated
| # push!(ratio_symrcm, nnz(ldlt_symrcm) / nnz_A) | ||
| # push!(ratio_classic, nnz(ldlt_classic) / nnz_A) | ||
|
|
||
| SUITE["no_ordering"][name] = @benchmarkable collect(1:size($A,1)) |
There was a problem hiding this comment.
Well, this line used to say nnz(tril(A)) but I think it should be nnz(tril(A, -1)) because we don't store the diagonal of the L factor. So we want to compare the number of nonzeros in L (which is strictly lower triangular) to the number of nonzeros in the strict lower triangle of A.
benchmark/benchmarks.jl
Outdated
| @@ -45,37 +43,39 @@ for subdir ∈ subdirs | |||
|
|
|||
| A = MatrixMarket.mmread(joinpath(iterpath, "K_0.mtx")) | |||
| b = readdlm(joinpath(iterpath, "rhs_0.rhs"), Float64)[:] | |||
| nnz_A = nnz(tril(A)) | |||
| nnz_A = nnz(tril(A, -1)) | |||
| n = size(A,1) | |||
| if size(A, 1) ≤ 25000 | |||
There was a problem hiding this comment.
Should I erase line 47?
There was a problem hiding this comment.
No. I mean to use n instead of size(A, 1) on line 48.
|
It's looking good now! At this point, the run_benchmarks.jl script is (almost) identical to that in LDLFactorizations.jl, and the send_comment_to_pr.jl and push_benchmarks.sh are identical. Can we think of how to put the common parts in another repo so we don't have to repeat them every time? |
|
Thank you @MonssafToukal ! |
No description provided.