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

Fix sparse array setindex(::Int, ::Vector) #43678

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

dkarrasch
Copy link
Member

Alternative fix of #43644. In comparison to #43650, this avoids a double reshape via vec-reshape, and catches the problematic case in a narrower fashion, thereby (hopefully) avoiding undesired side effects. Some little code rearrangement and removal of dead code follows in the second commit.

If we go with this one, then this closes #43644 and closes #43650.

@dkarrasch dkarrasch added kind:bugfix This change fixes an existing bug domain:arrays:sparse Sparse arrays labels Jan 6, 2022
@gbaraldi
Copy link
Member

gbaraldi commented Jan 6, 2022

What I'm not sure of is what behavior should we allow, there was a short discussion here of what is allowed in the setindex of an array.
https://julialang.slack.com/archives/C6GHSTN4T/p1641410970153500?thread_ts=1641410970.153500&cid=C6GHSTN4T

But basically this is allowed, even tho it errors on broadcasting operations.

 julia> z = zeros(Int,4,4)

 
 julia> z[3, :] = 10ones(2,2);
 
 julia> z
 4×4 Matrix{Int64}:
   1   1   0   0
   1   1   0   0
  10  10  10  10
   0   0   0   0

@dkarrasch
Copy link
Member Author

I see. That one is allowed here. As you say in the Slack thread, that (w/sh)ould be prohibited by Base.setindex_shape_check, right? I quickly checked, on your branch this example (with z = spzeros(Int, 4, 4)) leads to the original segfault.

@gbaraldi
Copy link
Member

gbaraldi commented Jan 6, 2022

That's the thing, when I made my branch I thought it errored and that only (1,n,1...) arrays were allowed, but apparently not.

Since that is the current behaviour of normal arrays I guess sparse arrays should follow it, but IMO that should be considered a bug. In fact if it were up to me setindex should only be allowed if the shapes matched or if RHS is a vector.

@dkarrasch
Copy link
Member Author

Shall we go with this one then @gbaraldi? The fix of the shape issue is independent and has to be made in a different place, such that this setindex! method will benefit automatically.

@gbaraldi
Copy link
Member

gbaraldi commented Jan 7, 2022

Yeah it's fine by me, it segfaulted before, so nobody will miss the weird behaviour anyway.

@dkarrasch dkarrasch merged commit dc61f29 into master Jan 7, 2022
@dkarrasch dkarrasch deleted the dk/sparse_setindex branch January 7, 2022 15:01
@gbaraldi
Copy link
Member

gbaraldi commented Jan 7, 2022

Can you add the backport labels so it gets backported? They were present in my original PR so I imagine they should be here too.

@dkarrasch dkarrasch added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Jan 7, 2022
@KristofferC KristofferC mentioned this pull request Jan 10, 2022
23 tasks
KristofferC pushed a commit that referenced this pull request Jan 10, 2022
@KristofferC KristofferC mentioned this pull request Jan 10, 2022
50 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
KristofferC pushed a commit that referenced this pull request Mar 15, 2022
KristofferC pushed a commit that referenced this pull request Mar 16, 2022
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label May 16, 2022
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays:sparse Sparse arrays kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sparse arrays segmentation fault when calling setindex(::Int, ::Vector) = ::Adjoint
3 participants