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

Overloading fill psparsematrix #34

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

amartinhuertas
Copy link
Collaborator

I think these changes will be needed by GridapDistributed.jl at some point

@amartinhuertas amartinhuertas changed the base branch from master to gridap_distributed October 19, 2021 04:17
@fverdugo
Copy link
Owner

fverdugo commented Oct 19, 2021

I always have the doubt if fill!(a,v) makes sense for sparse matrices for v!=0.

The SparseMatrixCSC has this behaviour. Note the difference between fill!(b,5) and fill!(c,0):

julia> using SparseArrays

julia> a = zeros(3,3);

julia> a[1,3]=3;

julia> a[3,2]=1;

julia> a[1,1]=3;

julia> a
3×3 Matrix{Float64}:
 3.0  0.0  3.0
 0.0  0.0  0.0
 0.0  1.0  0.0

julia> b = sparse(a)
3×3 SparseMatrixCSC{Float64, Int64} with 3 stored entries:
 3.0   ⋅   3.0
  ⋅    ⋅    ⋅ 
  ⋅   1.0   ⋅ 

julia> fill!(b,5)
3×3 SparseMatrixCSC{Float64, Int64} with 9 stored entries:
 5.0  5.0  5.0
 5.0  5.0  5.0
 5.0  5.0  5.0

julia> c = sparse(a)
3×3 SparseMatrixCSC{Float64, Int64} with 3 stored entries:
 3.0   ⋅   3.0
  ⋅    ⋅    ⋅ 
  ⋅   1.0   ⋅ 

julia> fill!(c,0)
3×3 SparseMatrixCSC{Float64, Int64} with 3 stored entries:
 0.0   ⋅   0.0
  ⋅    ⋅    ⋅ 
  ⋅   0.0   ⋅ 

@fverdugo fverdugo self-requested a review October 19, 2021 05:43
Copy link
Owner

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge this but we need to check that the fill value is == 0, otherwise we need to throw a "no implemented" error

@amartinhuertas
Copy link
Collaborator Author

I always have the doubt if fill!(a,v) makes sense for sparse matrices for v!=0.

There is a discussion in regards to this in JuliaLang/julia#17670

The SparseMatrixCSC has this behaviour. Note the difference between fill!(b,5) and fill!(c,0):

I did not know that. I think this is not mentioned in the link above, neither.

We can merge this but we need to check that the fill value is == 0, otherwise we need to throw a "no implemented" error

And what if you want to set all entries of the matrix to a given value different from zero? How do you do that?

@amartinhuertas
Copy link
Collaborator Author

And what if you want to set all entries of the matrix to a given value different from zero? How do you do that?

Ok, perhaps we should override the LinearAlgebra.fillstored! for PSparseMatrix better than Base.fill!. Agreed?

@fverdugo
Copy link
Owner

Ok, perhaps we should override the LinearAlgebra.fillstored! for PSparseMatrix better than Base.fill!. Agreed?

Yes! I was not aware of this function. Very usefull!

@amartinhuertas
Copy link
Collaborator Author

Yes! I was not aware of this function. Very usefull!

Ok. Done. The only inconvenience is that fillstored! is not exported from LinearAlgebra. Does it mean that it is more prone to change without notice?

@codecov-commenter
Copy link

Codecov Report

Merging #34 (3086f1d) into gridap_distributed (f4d2901) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           gridap_distributed      #34      +/-   ##
======================================================
+ Coverage               92.30%   92.34%   +0.04%     
======================================================
  Files                       8        8              
  Lines                    1962     1973      +11     
======================================================
+ Hits                     1811     1822      +11     
  Misses                    151      151              
Impacted Files Coverage Δ
src/Interfaces.jl 91.52% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d2901...3086f1d. Read the comment docs.

@fverdugo fverdugo merged commit 5908b30 into gridap_distributed Oct 19, 2021
@fverdugo fverdugo deleted the overloading_fill_psparsematrix branch October 19, 2021 09:47
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