Skip to content

Add option to decompress into a triangle only#83

Merged
gdalle merged 2 commits intomainfrom
gd/uplo
Aug 14, 2024
Merged

Add option to decompress into a triangle only#83
gdalle merged 2 commits intomainfrom
gd/uplo

Conversation

@gdalle
Copy link
Copy Markdown
Member

@gdalle gdalle commented Aug 14, 2024

  • Add uplo = :U/:L/:UL parameter to decompress! and decompress_single_color! to allow picking the triangle for symmetric matrices
  • Implement the corresponding methods
  • Add tests for full decompression and for single color, structure into subtests

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (87508b9) to head (6c0f721).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage   99.85%   99.86%           
=======================================
  Files          10       10           
  Lines         675      715   +40     
=======================================
+ Hits          674      714   +40     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gdalle gdalle merged commit 0f00909 into main Aug 14, 2024
@gdalle gdalle deleted the gd/uplo branch August 14, 2024 15:01
@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 14, 2024

@amontoison you asked why uplo is not a kwarg. The reason is that I want to be able to dispatch on whether it is present or not, and you cannot dispatch on kwargs (not even on their presence or absence).

My reasoning is that between the following methods, the first one is more efficient (leads to predictable memory accesses) and should thus be preferred when uplo is not specified. What do you think?

function decompress!(
    A::SparseMatrixCSC{R}, B::AbstractMatrix{R}, result::StarSetColoringResult
) where {R<:Real}
    @compat (; S, compressed_indices) = result
    check_same_pattern(A, S)
    nzA = nonzeros(A)
    for k in eachindex(nzA, compressed_indices)
        nzA[k] = B[compressed_indices[k]]
    end
    return A
end

function decompress!(
    A::SparseMatrixCSC{R}, B::AbstractMatrix{R}, result::StarSetColoringResult, uplo::Symbol
) where {R<:Real}
    @compat (; S, compressed_indices) = result
    check_same_pattern(A, S)
    rvA = rowvals(A)
    nzA = nonzeros(A)
    for j in axes(S, 2)
        for k in nzrange(S, j)
            i = rvA[k]
            if in_triangle(i, j, uplo)
                nzA[k] = B[compressed_indices[k]]
            end
        end
    end
    return A
end

@amontoison
Copy link
Copy Markdown
Collaborator

amontoison commented Aug 14, 2024

You will not gain a lot if you do that:

function decompress!(
    A::SparseMatrixCSC{R}, B::AbstractMatrix{R}, result::StarSetColoringResult; uplo::Symbol=:F
) where {R<:Real}
    @compat (; S, compressed_indices) = result
    check_same_pattern(A, S)
    nzA = nonzeros(A)
    if uplo == :F
      for k in eachindex(nzA, compressed_indices)
          nzA[k] = B[compressed_indices[k]]
      end
    else
      rvA = rowvals(A)
      for j in axes(S, 2)
        for k in nzrange(S, j)
          i = rvA[k]
          if in_triangle(i, j, uplo)
            nzA[k] = B[compressed_indices[k]]
          end
        end
      end
    end
    return A
end

@amontoison
Copy link
Copy Markdown
Collaborator

amontoison commented Aug 14, 2024

The issue is that sometimes uplo is a kwarg and sometimes not.
I prefer your approach but we should do that for all decompress!.

@gdalle
Copy link
Copy Markdown
Member Author

gdalle commented Aug 14, 2024

You will not gain a lot if you do that:

True but with dispatch I avoid compiling the useless part of the code.

The issue is that sometimes uplo is a kwarg and sometimes not.

There was one spot where it was (erroneously) a kwarg, I fixed it in #85. I think now it should be a positional argument everywhere.

@amontoison
Copy link
Copy Markdown
Collaborator

You code too fast for me. It's perfect in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an option to just decompress in one triangle for star and acyclic coloring

2 participants