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

Bug in unaliascopy for a SubArray with AbstractRange indices #54100

Closed
jishnub opened this issue Apr 16, 2024 · 1 comment · Fixed by #54102
Closed

Bug in unaliascopy for a SubArray with AbstractRange indices #54100

jishnub opened this issue Apr 16, 2024 · 1 comment · Fixed by #54102
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version

Comments

@jishnub
Copy link
Contributor

jishnub commented Apr 16, 2024

julia> struct MyStepRange{T} <: OrdinalRange{T,T}
               r::StepRange{T,T}
       end

julia> for f in (:first, :last, :step, :length, :size)
               @eval Base.$f(r::MyStepRange) = $f(r.r)
       end

julia> Base.getindex(r::MyStepRange, i::Int) = r.r[i]

julia> A = rand(6);

julia> V = view(A, 2:2:4)
2-element view(::Vector{Float64}, 2:2:4) with eltype Float64:
 0.02519989368909037
 0.5834755544565842

julia> V2 = view(A, MyStepRange(2:2:4))
2-element view(::Vector{Float64}, 2:2:4) with eltype Float64:
 0.02519989368909037
 0.5834755544565842

julia> V == V2
true

julia> Base.unaliascopy(V2) # incorrect
2-element view(::Vector{Float64}, 2:2:4) with eltype Float64:
 6.9259266686866e-310
 0.02519989368909037

julia> Base.unaliascopy(V) # correct
2-element view(::Vector{Float64}, 1:1:2) with eltype Float64:
 0.02519989368909037
 0.5834755544565842

This seems to arise from

julia/base/subarray.jl

Lines 113 to 119 in d8b9810

function unaliascopy(V::SubArray{T,N,A,I,LD}) where {T,N,A<:Array,I<:Tuple{Vararg{Union{ScalarIndex,AbstractRange{<:ScalarIndex},Array{<:Union{ScalarIndex,AbstractCartesianIndex}}}}},LD}
dest = Array{T}(undef, _trimmedshape(V.indices...))
trimmedpind = _trimmedpind(V.indices...)
vdest = trimmedpind isa Tuple{Vararg{Union{Slice,Colon}}} ? dest : view(dest, trimmedpind...)
copyto!(vdest, view(V, _trimmedvind(V.indices...)...))
SubArray{T,N,A,I,LD}(dest, map(_trimmedindex, V.indices), 0, Int(LD))
end

where the offset and stride are being hard-coded instead of being computed.

Version:

julia> versioninfo()
Julia Version 1.12.0-DEV.343
Commit 98f47474d83 (2024-04-15 04:40 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, tigerlake)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)
Environment:
  LD_LIBRARY_PATH = :/usr/lib/x86_64-linux-gnu/gtk-3.0/modules
  JULIA_EDITOR = subl

This issue exists on v"1.11.0-beta1" as well. The result is correct on v"1.10.2".

@jishnub jishnub added bug Indicates an unexpected problem or unintended behavior arrays [a, r, r, a, y, s] regression Regression in behavior compared to a previous version labels Apr 16, 2024
@jishnub
Copy link
Contributor Author

jishnub commented Apr 16, 2024

Also note that maximum in _trimmedshape will throw for an empty range unless an init is specified. This currently causes a test failure in FillArrays.jl.

N5N3 pushed a commit that referenced this issue Apr 16, 2024
Fix #54100 by computing the
stride and offset explicitly. This is unlikely to be a performance
concern, so we don't need to hard-code this.
KristofferC pushed a commit that referenced this issue Apr 17, 2024
Fix #54100 by computing the
stride and offset explicitly. This is unlikely to be a performance
concern, so we don't need to hard-code this.

(cherry picked from commit 159f4d7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant