JuliaSparse / SparseArrays.jl

SparseArrays.jl is a Julia stdlib
https://sparsearrays.juliasparse.org/
Other
92 stars 52 forks source link

Sparse versions of repeat for matrices and vectors #532

Closed mjacobse closed 6 months ago

mjacobse commented 7 months ago

Calling repeat for sparse matrices or vectors currently calls the Base function, which ends up building the result rather inefficiently with indexing brackets.

This would add sparse versions that use the info that inputs are in CSC format to provide more efficient implementations. To keep it simple it is limited to the straightforward cases of outer repetition along the first two dimensions. Row-wise repetition is intentionally kept close to the implementation for vcat, also using its helper function stuffcol! (with slight modifications). Column-wise repetition is kept simple by deferring to Base.repeat on the colptr, rowval, and nzval components of the CSC format.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.07%. Comparing base (33fbc75) to head (6db2712). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #532 +/- ## ========================================== + Coverage 76.42% 84.07% +7.64% ========================================== Files 12 12 Lines 8969 9068 +99 ========================================== + Hits 6855 7624 +769 + Misses 2114 1444 -670 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SobhanMP commented 7 months ago

was vcat using the wrong length?

mjacobse commented 7 months ago

was vcat using the wrong length?

Are you referring to col_length and the change in 60eff16? No, in effect it was used correctly. Basically the helper stuffcol! iterates from 0:(n-1) where n is the number of nonzeros in the column. Before 60eff16, it expected you to pass in n-1 for the argument called col_length and then iterated 0:col_length. After 60eff16 it expects you to pass in n for the argument called col_length and then iterates 0:(col_length-1). So the behavior does not change, but to my mind it is much clearer and consistent with the name of the argument. And as I intended to reuse the function in the new repeat, I thought it would be better to make this clarifying change instead of reproducing the somewhat confusing usage.

ViralBShah commented 7 months ago

@SobhanMP Can you add your review to this PR?

mjacobse commented 6 months ago

Thanks for the review! Anything left that I can do to help moving this forward?

ViralBShah commented 6 months ago

@SobhanMP Good to merge?

SobhanMP commented 6 months ago

yes, sorry I was behind a deadline :sweat_smile: @mjacobse, thanks for the code.