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

Correctly set zeros with fill!(::SubArray) and fix its return value #433

Merged
merged 4 commits into from
Sep 5, 2023

Conversation

sunoru
Copy link
Contributor

@sunoru sunoru commented Sep 1, 2023

This PR includes two fixes:

1

It is a simple obvious fix to the problem when one tries to do something like the following

using SparseArrays, StaticArrays
const Vec2 = SVector{2, Float64}
A = rand(Vec2, 2, 2) |> sparse
fill!(view(A, :, :), zero(Vec2))

Without the fix, it throws

ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type SVector{2, Float64}

Closest candidates are:
  convert(::Type{T}, ::Factorization) where T<:AbstractArray
   @ LinearAlgebra /usr/share/julia/stdlib/v1.9/LinearAlgebra/src/factorization.jl:59
  convert(::Type{SVector{N, T}}, ::CartesianIndex{N}) where {N, T}
   @ StaticArrays ~/.julia/packages/StaticArrays/jA1zK/src/SVector.jl:13
  convert(::Type{SA}, ::Tuple) where SA<:StaticArray
   @ StaticArrays ~/.julia/packages/StaticArrays/jA1zK/src/convert.jl:179
  ...

Stacktrace:
 [1] setindex!(A::Vector{SVector{2, Float64}}, x::Int64, i1::Int64)
   @ Base ./array.jl:969
 [2] _spsetz_setindex!
   @ /usr/share/julia/stdlib/v1.9/SparseArrays/src/sparsematrix.jl:3033 [inlined]
 [3] fill!(V::SubArray{SVector{2, Float64}, 1, SparseMatrixCSC{SVector{2, Float64}, Int64}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, false}, x::SVector{2, Float64})
   @ SparseArrays /usr/share/julia/stdlib/v1.9/SparseArrays/src/sparsematrix.jl:3001
 [4] top-level scope
   @ REPL[61]:1

2

fill!(A, ...) should always return A, but without the fix, it returns A.parent for sub arrays of the sparse arrays, if zeros are being set.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #433 (5b32f28) into main (4e6776a) will increase coverage by 0.78%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   92.42%   93.20%   +0.78%     
==========================================
  Files          12       12              
  Lines        7667     7668       +1     
==========================================
+ Hits         7086     7147      +61     
+ Misses        581      521      -60     
Files Changed Coverage Δ
src/sparsematrix.jl 95.68% <100.00%> (+<0.01%) ⬆️

... and 2 files with indirect coverage changes

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

@dkarrasch
Copy link
Member

Do you think you could add a test for this case? Maybe there is some number type that is not convertible to from an integer 0? Perhaps some unitful number type should work?

@sunoru
Copy link
Contributor Author

sunoru commented Sep 1, 2023

OK I will try to add the tests.

Actually I just found another issue related to the fill! function for sub arrays that it should always returns the given array (the first argument) instead of the parent of that array. Should I submit another PR or can I also fix it here since I think they are both small fixes?

@sunoru
Copy link
Contributor Author

sunoru commented Sep 1, 2023

There is no number type except Irrational that is not convertible to from an integer 0, but it also doesn't have a zero value.

The only common use case I can imagine is for SVectors, but for the test purpose I can just declare a custom struct.

@dkarrasch
Copy link
Member

Actually I just found another issue related to the fill! function for sub arrays

Fix here, but for easier review, please submit first a commit with a test that currently fails, and then another commit with the fix.

@sunoru sunoru changed the title Setting zero to subarrays of a sparse non-number matrix. Correctly set zeros with fill!(::SubArray) and fix its return value Sep 4, 2023
@dkarrasch dkarrasch merged commit 2fae1a1 into JuliaSparse:main Sep 5, 2023
@sunoru sunoru deleted the patch-1 branch September 5, 2023 18:54
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.

2 participants