-
-
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 stride(A, i)
for 0-dim inputs
#44090
Conversation
Do we prefer
over
? |
I have no idea. For me we can return any value if |
base/abstractarray.jl
Outdated
@@ -546,7 +546,7 @@ julia> stride(A,3) | |||
function stride(A::AbstractArray, k::Integer) | |||
st = strides(A) | |||
k ≤ ndims(A) && return st[k] | |||
return sum(st .* size(A)) | |||
return ndims(A) == 0 ? 1 : sum(st .* size(A)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a very low-level function to be calling broadcast and reduce. Should probably be a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest restoring this function:
function _stride(a, i)
if i > ndims(a)
return length(a)
end
s = 1
for n = 1:(i-1)
s *= size(a, n)
end
return s
end
julia/base/reinterpretarray.jl
Lines 162 to 171 in dd0c14b
function _stride(a, i) | |
if i > ndims(a) | |
return length(a) | |
end | |
s = 1 | |
for n = 1:(i-1) | |
s *= size(a, n) | |
end | |
return s | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The orginal version doesn't thrown a error for stride(a, -1)
.
On 1.7, we have
julia> stride([], -1)
1
julia> stride(view([1],1:1), -1)
ERROR: BoundsError: attempt to access Tuple{Int64} at index [-1]
Stacktrace:
[1] getindex
@ .\tuple.jl:29 [inlined]
[2] stride(V::SubArray{Int64, 1, Vector{Int64}, Tuple{UnitRange{Int64}}, true}, d::Int64)
@ Base .\subarray.jl:358
[3] top-level scope
@ REPL[2]:1
I think it's meaningful to keep error messages' consistency. The easist way is to follow the implement in general fallback.
(And they should have no performance difference)
@JeffBezanson , @N5N3 . I'm confused. I get a distinct result with this new implementation. Julia 1.7.2: julia> stride(zeros(5,3,2),1)
1
julia> stride(zeros(5,3,2),2)
5
julia> stride(zeros(5,3,2),3)
15
julia> stride(zeros(5,3,2),4)
30 This function: julia> function new_stride(A::AbstractArray, k::Integer)
st = strides(A)
k ≤ ndims(A) && return st[k]
ndims(A) == 0 && return 1
sz = size(A)
s = st[1] * sz[1]
for i in 2:ndims(A)
s += st[i] * sz[i]
end
return s
end
new_stride (generic function with 1 method)
julia> new_stride(zeros(5,3,2),1)
1
julia> new_stride(zeros(5,3,2),2)
5
julia> new_stride(zeros(5,3,2),3)
15
julia> new_stride(zeros(5,3,2),4)
50 Why are we summing?
|
We only do sum for non-dense array. see #35929 (comment) for more details. |
I'm not sure I follow the logic either for |
I guess yes. Although it's correctness is also fragile as stride could |
This PR recover the orignal behavior of
stride
forUnion{DenseArray,StridedReshapedArray,StridedReinterpretArray}
.Besides,
stride(a::AbstractArray{<:Any, 0}, k)
now always return1
ifk >= 1
andstrides
is defined.Test added. (Close #44087)