-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
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. 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 |
5b149c0
to
854f936
Compare
I see. That one is allowed here. As you say in the Slack thread, that (w/sh)ould be prohibited by |
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. |
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 |
Yeah it's fine by me, it segfaulted before, so nobody will miss the weird behaviour anyway. |
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. |
(cherry picked from commit dc61f29)
(cherry picked from commit dc61f29)
(cherry picked from commit dc61f29)
(cherry picked from commit dc61f29)
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.