Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decrement should always return a vector #241

Merged
merged 3 commits into from
Sep 6, 2022
Merged

decrement should always return a vector #241

merged 3 commits into from
Sep 6, 2022

Conversation

SobhanMP
Copy link
Member

@SobhanMP SobhanMP commented Aug 30, 2022

we only use it when it returns a vector, so i think it's good idea for AbstractMatrixCSCs that don't use Vectors (like Fixed)

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Merging #241 (d239e11) into main (43b4d01) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
- Coverage   91.68%   91.62%   -0.07%     
==========================================
  Files          12       12              
  Lines        7302     7305       +3     
==========================================
- Hits         6695     6693       -2     
- Misses        607      612       +5     
Impacted Files Coverage Δ
src/readonly.jl 72.22% <ø> (-1.47%) ⬇️
src/SparseArrays.jl 85.00% <100.00%> (-15.00%) ⬇️
src/solvers/umfpack.jl 87.70% <0.00%> (-0.41%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SobhanMP
Copy link
Member Author

@Wimmerer do you think this is a good idea?

@rayegun
Copy link
Member

rayegun commented Aug 30, 2022

I'm not super fond of it since most sparse matrices are not hypersparse.

But I think it is correct.

@SobhanMP
Copy link
Member Author

SobhanMP commented Aug 30, 2022

this is an internal only used by CHOLMOD and lu! to convert to one based indexing rigth?

@SobhanMP SobhanMP changed the title decremenet should always return a vector decrement should always return a vector Aug 30, 2022
@rayegun
Copy link
Member

rayegun commented Aug 30, 2022

Oh wrong issue, let me look at this

@SobhanMP
Copy link
Member Author

SobhanMP commented Aug 30, 2022

The issue that for types that don't have a vector colptr, lu/qr wont work, this is one of the problematic parts

@rayegun
Copy link
Member

rayegun commented Aug 30, 2022

A very quick and dirty benchmark shows that is ~1.3x slower. Do you see that as well?

@SobhanMP
Copy link
Member Author

SobhanMP commented Aug 30, 2022

julia> dec_alt(x) = SparseArrays.decrement!(copy(x))
dec_alt (generic function with 1 method)

julia> dec_alt1(x) = SparseArrays.decrement!(Array(x))
dec_alt1 (generic function with 1 method)

julia> dec_alt2(x) = let y = Array{eltype(x),ndims(x)}(undef, size(x)...)
           y .= x .- oneunit(eltype(x))
           y
       end
dec_alt2 (generic function with 1 method)

julia> dec_alt3(x) = let y = Vector{eltype(x)}(undef, size(x)...)
           y .= x .- oneunit(eltype(x))
           y
       end
dec_alt3 (generic function with 1 method)

julia> dec_alt4(x) = let y = Vector(x)
           y .= y .- oneunit(eltype(x))
           y
       end
dec_alt4 (generic function with 1 method)

julia> dec_alt5(x) = let y = Vector(x)
           for i in eachindex(y, x)
               y[i] -= oneunit(eltype(x))
           end
           y
       end
dec_alt5 (generic function with 1 method)

julia> dec_alt6(x) = let y = Vector{eltype(x)}(undef, length(x))
           for i in eachindex(y, x)
               y[i] = x[i] - oneunit(eltype(x))
           end
           y
       end
dec_alt6 (generic function with 1 method)

julia> dec_alt7(x) = let y = Array{eltype(x),ndims(x)}(undef, size(x)...)
           for i in eachindex(y, x)
               y[i] = x[i] - oneunit(eltype(x))
           end
           y
       end
dec_alt7 (generic function with 1 method)

julia> dec_alt41(x) = let y = Array(x)
           y .= y .- oneunit(eltype(x))
           y
       end
dec_alt41 (generic function with 1 method)

julia> aa = [dec_alt,dec_alt1,dec_alt2,dec_alt3,dec_alt4,dec_alt41,dec_alt5,dec_alt6,dec_alt7]
9-element Vector{Function}:
 dec_alt (generic function with 1 method)
 dec_alt1 (generic function with 1 method)
 dec_alt2 (generic function with 1 method)
 dec_alt3 (generic function with 1 method)
 dec_alt4 (generic function with 1 method)
 dec_alt41 (generic function with 1 method)
 dec_alt5 (generic function with 1 method)
 dec_alt6 (generic function with 1 method)
 dec_alt7 (generic function with 1 method)

julia> for i in aa, j in aa
           @assert i(a) == j(a)
       end

julia> @benchmark dec_alt($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):  5.532 μs  343.084 μs  ┊ GC (min  max):  0.00%  92.03%
 Time  (median):     6.121 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   9.884 μs ±  14.471 μs  ┊ GC (mean ± σ):  11.99% ±  8.91%

  █▅▄▃▃▃▂▂▁▁▁▁  ▁▄▂                                           ▁
  ██████████████████▇▇▇▇▆▆▆▆▆▆▆▄▅▃▅▅▅▄▄▅▃▅▁▁▁▃▁▃▁▁▁▁▁▃▁▁▁▁▃▁▄ █
  5.53 μs      Histogram: log(frequency) by time      63.1 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt1($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):  5.533 μs  239.264 μs  ┊ GC (min  max): 0.00%  92.10%
 Time  (median):     5.878 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   7.555 μs ±   9.059 μs  ┊ GC (mean ± σ):  9.94% ±  8.97%

  █▄▃▂▂▂▁▁ ▁▂▁▁                                               ▁
  ██████████████▇▆▃▄▅▇▆▇▃▁▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▄ █
  5.53 μs      Histogram: log(frequency) by time      52.8 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt2($a)
BenchmarkTools.Trial: 10000 samples with 7 evaluations.
 Range (min  max):   4.380 μs  442.384 μs  ┊ GC (min  max):  0.00%  91.19%
 Time  (median):     13.540 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   16.176 μs ±  24.474 μs  ┊ GC (mean ± σ):  13.95% ±  9.79%

  ▅█▁    ▁▁▁                                                    
  ███▆▅▆▇███▇▅▄▃▃▂▂▂▂▂▂▂▂▂▁▂▁▁▁▁▁▁▁▁▂▁▁▁▂▂▁▂▂▁▂▂▁▁▁▁▂▁▂▁▂▂▁▂▂▂ ▃
  4.38 μs         Histogram: frequency by time          106 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt3($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):  3.812 μs  272.141 μs  ┊ GC (min  max):  0.00%  91.12%
 Time  (median):     5.442 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   7.400 μs ±   9.344 μs  ┊ GC (mean ± σ):  11.08% ±  9.15%

  ▃█▅▄▄▄▃▃▂▂▁▁▁▁                                              ▁
  ███████████████▆▆▅▄▅▃▄▁▃▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▅ █
  3.81 μs      Histogram: log(frequency) by time      57.3 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt4($a)
BenchmarkTools.Trial: 10000 samples with 8 evaluations.
 Range (min  max):  2.998 μs  390.348 μs  ┊ GC (min  max):  0.00%  86.35%
 Time  (median):     3.507 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   6.478 μs ±  13.019 μs  ┊ GC (mean ± σ):  17.84% ± 10.62%

  █▅▅▄▄▃▃▂▃▃▂▂▂▁                                              ▂
  ████████████████▇▇▇▇▇▇▆▅▅▄▁▅▁▃▄▃▁▃▃▃▁▁▁▁▃▃▃▃▃▄▄▄▃▃▃▄▅▄▅▄▁▃▅ █
  3 μs         Histogram: log(frequency) by time      52.2 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt5($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):   5.532 μs  429.331 μs  ┊ GC (min  max):  0.00%  94.89%
 Time  (median):      5.869 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   10.103 μs ±  16.150 μs  ┊ GC (mean ± σ):  12.99% ±  8.92%

  █▅▃▂▁▂▁        ▄▄▂                                           ▁
  ███████▇▇▇▇▆▅▃▇███▇▇▆▆▅▅▅▅▆▅▄▅▄▄▄▄▄▄▁▅▄▄▃▃▃▃▄▁▁▁▁▃▃▁▁▁▃▁▁▁▃▄ █
  5.53 μs       Histogram: log(frequency) by time      63.5 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt6($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):  4.325 μs  238.120 μs  ┊ GC (min  max):  0.00%  92.51%
 Time  (median):     5.529 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   7.491 μs ±   9.086 μs  ┊ GC (mean ± σ):  10.81% ±  9.15%

  ▇█▄▅▄▃▃▂▁▁▁▁▁▁▁▁                                            ▂
  ████████████████▇▆▅▃▃▁▃▁▃▃▃▁▁▁▁▁▁▁▁▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▄▄ █
  4.32 μs      Histogram: log(frequency) by time      61.9 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt7($a)
BenchmarkTools.Trial: 10000 samples with 6 evaluations.
 Range (min  max):  4.203 μs  169.380 μs  ┊ GC (min  max):  0.00%  89.03%
 Time  (median):     5.522 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   7.203 μs ±   8.376 μs  ┊ GC (mean ± σ):  10.83% ±  9.11%

  ▅█▅▆▄▃▂▁▁▁                                                  ▁
  ████████████▇▅▅▇▆▁▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▅ █
  4.2 μs       Histogram: log(frequency) by time      59.9 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

julia> @benchmark dec_alt41($a)
BenchmarkTools.Trial: 10000 samples with 8 evaluations.
 Range (min  max):  2.998 μs  358.718 μs  ┊ GC (min  max):  0.00%  93.88%
 Time  (median):     6.672 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   9.759 μs ±  19.404 μs  ┊ GC (mean ± σ):  18.92% ± 10.61%

  █                                                            
  █▄▄▄▄▅▅▄▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▂▁▂▂▂▁▁▂▂▂▁▁▂▂▂▁▁▁▁▂▁▁▁▁▂▁▁▁▂ ▂
  3 μs            Histogram: frequency by time        78.9 μs <

 Memory estimate: 78.17 KiB, allocs estimate: 2.

@SobhanMP
Copy link
Member Author

i think version 4 is good, maybe there is no need to bother with weird indexes.

@SobhanMP SobhanMP requested a review from rayegun September 1, 2022 02:59
@SobhanMP
Copy link
Member Author

SobhanMP commented Sep 6, 2022

@Wimmerer ping

@SobhanMP SobhanMP merged commit d88be9f into main Sep 6, 2022
@SobhanMP SobhanMP deleted the so/decrement branch September 6, 2022 17:38
fredrikekre added a commit to JuliaLang/julia that referenced this pull request Sep 16, 2022
This patch updates SparseArrays. In particular it contains
JuliaSparse/SparseArrays.jl#260 which is
necessary to make progress in #46759.

All changes:
 - Fix direction of circshift (JuliaSparse/SparseArrays.jl#260)
 - Fix `vcat` of sparse vectors with numbers (JuliaSparse/SparseArrays.jl#253)
 - decrement should always return a vector (JuliaSparse/SparseArrays.jl#241)
 - change order of arguments in fkeep, fix bug with fixed elements (JuliaSparse/SparseArrays.jl#240)
 - Sparse matrix/vectors with fixed sparsity pattern. (JuliaSparse/SparseArrays.jl#201)
fredrikekre added a commit to JuliaLang/julia that referenced this pull request Sep 16, 2022
This patch updates SparseArrays. In particular it contains
JuliaSparse/SparseArrays.jl#260 which is
necessary to make progress in #46759.

All changes:
 - Remove fkeep! from the documentation (JuliaSparse/SparseArrays.jl#261)
 - Fix direction of circshift (JuliaSparse/SparseArrays.jl#260)
 - Fix `vcat` of sparse vectors with numbers (JuliaSparse/SparseArrays.jl#253)
 - decrement should always return a vector (JuliaSparse/SparseArrays.jl#241)
 - change order of arguments in fkeep, fix bug with fixed elements (JuliaSparse/SparseArrays.jl#240)
 - Sparse matrix/vectors with fixed sparsity pattern. (JuliaSparse/SparseArrays.jl#201)
fredrikekre added a commit to JuliaLang/julia that referenced this pull request Sep 22, 2022
This patch updates SparseArrays. In particular it contains
JuliaSparse/SparseArrays.jl#260 which is
necessary to make progress in #46759.

All changes:
 - Fix ambiguities with Base. (JuliaSparse/SparseArrays.jl#268)
 - add == for vectors (JuliaSparse/SparseArrays.jl#248)
 - add undef initializers (JuliaSparse/SparseArrays.jl#263)
 - Make sure reductions benefit from sparsity (JuliaSparse/SparseArrays.jl#244)
 - Remove fkeep! from the documentation (JuliaSparse/SparseArrays.jl#261)
 - Fix direction of circshift (JuliaSparse/SparseArrays.jl#260)
 - Fix `vcat` of sparse vectors with numbers (JuliaSparse/SparseArrays.jl#253)
 - decrement should always return a vector (JuliaSparse/SparseArrays.jl#241)
 - change order of arguments in fkeep, fix bug with fixed elements (JuliaSparse/SparseArrays.jl#240)
 - Sparse matrix/vectors with fixed sparsity pattern. (JuliaSparse/SparseArrays.jl#201)
fredrikekre added a commit to JuliaLang/julia that referenced this pull request Sep 27, 2022
This patch updates SparseArrays. In particular it contains
JuliaSparse/SparseArrays.jl#260 which is
necessary to make progress in #46759.

All changes:
 - Fix ambiguities with Base. (JuliaSparse/SparseArrays.jl#268)
 - add == for vectors (JuliaSparse/SparseArrays.jl#248)
 - add undef initializers (JuliaSparse/SparseArrays.jl#263)
 - Make sure reductions benefit from sparsity (JuliaSparse/SparseArrays.jl#244)
 - Remove fkeep! from the documentation (JuliaSparse/SparseArrays.jl#261)
 - Fix direction of circshift (JuliaSparse/SparseArrays.jl#260)
 - Fix `vcat` of sparse vectors with numbers (JuliaSparse/SparseArrays.jl#253)
 - decrement should always return a vector (JuliaSparse/SparseArrays.jl#241)
 - change order of arguments in fkeep, fix bug with fixed elements (JuliaSparse/SparseArrays.jl#240)
 - Sparse matrix/vectors with fixed sparsity pattern. (JuliaSparse/SparseArrays.jl#201)
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.

3 participants