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.

Sign up for GitHub

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

Jump to bottom

Use _setindex! to fix DimensionMismatch in sparse assignment, Fixes #28963 #30507

Merged
merged 1 commit into from
Dec 26, 2018
Merged

Use _setindex! to fix DimensionMismatch in sparse assignment, Fixes #28963 #30507

merged 1 commit into from
Dec 26, 2018

Conversation

raghav9-97
Copy link
Contributor

Fixes #28963.

@ViralBShah ViralBShah requested a review from mbauman December 25, 2018 07:17
@ViralBShah ViralBShah added the sparse Sparse arrays label Dec 25, 2018
@raghav9-97
Copy link
Contributor Author

Please review this PR.

@ViralBShah
Copy link
Member

A lot of people are on vacation this week. Hence reviews are likely to be slow.

Also may I request you to make the PR title and commit have a bit more detail about the issue in addition to the number?

@raghav9-97
Copy link
Contributor Author

raghav9-97 commented Dec 26, 2018 •
edited
Loading

@ViralBShah Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch, Fixes#28963 is this good enough as a commit message and PR title?

@ViralBShah
Copy link
Member

ViralBShah commented Dec 26, 2018 •
edited
Loading 

Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch in assignment
Fixes #28963

There can be many places where dimension mismatch happens. It is important to mention that it is fixing the mismatch in assignment.

@ViralBShah
Copy link
Member

I've reviewed this and am happy to merge once the commit message and PR titles are updated with a little more info as above.

@raghav9-97 raghav9-97 changed the title Fixed DimensionMismatch for SparseMatrixCSC Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch in assignment Fixes #28963 Dec 26, 2018
@raghav9-97 raghav9-97 changed the title Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch in assignment Fixes #28963 Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch in assignment, Fixes #28963 Dec 26, 2018
@ViralBShah ViralBShah changed the title Used _setindex! for checking SparseMatrices compatibility fixing DimensionMismatch in assignment, Fixes #28963 Use _setindex! to fix DimensionMismatch in sparse assignment, Fixes #28963 Dec 26, 2018
@ViralBShah ViralBShah merged commit b73a746 into JuliaLang:master Dec 26, 2018
@raghav9-97
Copy link
Contributor Author

Thanks @ViralBShah for helping out.

@raghav9-97 raghav9-97 deleted the sparsematrix branch December 26, 2018 17:31
@ViralBShah
Copy link
Member

ViralBShah commented Dec 26, 2018 •
edited
Loading

@fredrikekre are we going to backport to 1.0 as well? I have a bunch of other PRs merged that should also be backported.

@fredrikekre
Copy link
Member

Yes.

staticfloat pushed a commit that referenced this pull request Dec 30, 2018
@raghav9-97 @staticfloat
Fix DimensionMismatch in SparseMatrixCSC assignment (#30507)
7d81a90 
Use _setindex! to fix DimensionMismatch in sparse assignment
Fixes #28963
KristofferC pushed a commit that referenced this pull request Dec 30, 2018
@raghav9-97 @KristofferC
Fix DimensionMismatch in SparseMatrixCSC assignment (#30507)
ded25d3 
Use _setindex! to fix DimensionMismatch in sparse assignment
Fixes #28963

(cherry picked from commit b73a746)
@KristofferC KristofferC mentioned this pull request Dec 30, 2018
Closed
53 tasks
@mbauman mbauman added the bugfix This change fixes an existing bug label Dec 31, 2018
staticfloat pushed a commit that referenced this pull request Jan 4, 2019
@raghav9-97 @staticfloat
Fix DimensionMismatch in SparseMatrixCSC assignment (#30507)
11ec344 
Use _setindex! to fix DimensionMismatch in sparse assignment
Fixes #28963
@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call backport 1.1 and removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019
@StefanKarpinski StefanKarpinski added backport 1.0 triage This should be discussed on a triage call and removed triage This should be discussed on a triage call backport 1.0 labels Jan 31, 2019
@JeffBezanson JeffBezanson removed triage This should be discussed on a triage call triage backport pending 1.0 labels Jan 31, 2019
mbauman
mbauman reviewed Jan 31, 2019
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl
@@ -2575,6 +2575,7 @@ function setindex!(A::SparseMatrixCSC{Tv,Ti}, V::AbstractVecOrMat, Ix::Union{Int
@assert !has_offset_axes(A, V, Ix, Jx)
(I, J) = Base.ensure_indexable(to_indices(A, (Ix, Jx)))
checkbounds(A, I, J)
Base._setindex!(IndexStyle(A), A, V, to_indices(A, (Ix, Jx))...)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this just assign into the array twice? Once with the generic (slow) dense version, and then once with the optimized sparse version? We just want to be checking the shape of V, no?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed: before:

julia> A = spzeros(1000,1000)
1000×1000 SparseMatrixCSC{Float64,Int64} with 0 stored entries

julia> @btime $A[:,:] = $(spzeros(1000, 1000))
  5.557 μs (5 allocations: 8.39 KiB)

After:

julia> @btime $A[:,:] = $(spzeros(1000, 1000))
  9.545 ms (5 allocations: 8.39 KiB)

Copy link
Member

Choose a reason for hiding this comment

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

#30923

mbauman added a commit that referenced this pull request Jan 31, 2019
@mbauman
Fixup #30507, check indexed assignment size for SparseMatrix
c0922fc 
without assigning into the array twice.
StefanKarpinski pushed a commit that referenced this pull request Feb 4, 2019
@mbauman @StefanKarpinski
Fixup #30507, check indexed assignment size for SparseMatrix (#30923)
a5f680f 
without assigning into the array twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman mbauman left review comments

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug sparse Sparse arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@raghav9-97 @ViralBShah @fredrikekre @mbauman @StefanKarpinski @JeffBezanson