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

1.5: Surprising new pointer behaviour with adjoints and views #36402

Closed
dlfivefifty opened this issue Jun 23, 2020 · 1 comment · Fixed by #36405
Closed

1.5: Surprising new pointer behaviour with adjoints and views #36402

dlfivefifty opened this issue Jun 23, 2020 · 1 comment · Fixed by #36405
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@dlfivefifty
Copy link
Contributor

This seems wrong to me:

julia> A = randn(5,3);

julia> V = view(A',:,2:4)
3×3 view(::Adjoint{Float64,Array{Float64,2}}, :, 2:4) with eltype Float64:
 -0.6594    -0.124149   0.492452
  1.20338    0.224742  -0.0431509
 -0.425888   0.725104   0.976229

julia> Base.unsafe_load(pointer(V))
0.49245221105087544

I expected it to return the pointer to V[1,1], not V[1,end].

Related to #35929

@mbauman

@mbauman
Copy link
Member

mbauman commented Jun 23, 2020

Ah, yes. Our SubArray is implicitly assuming strided parents are both contiguous and column-major:

julia/base/subarray.jl

Lines 401 to 402 in b594172

unsafe_convert(::Type{Ptr{T}}, V::SubArray{T,N,P,<:Tuple{Vararg{RangeIndex}}}) where {T,N,P} =
unsafe_convert(Ptr{T}, V.parent) + (first_index(V)-1)*sizeof(T)

That needs to be doing a strides computation.

@mbauman mbauman added arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior labels Jun 23, 2020
@JeffBezanson JeffBezanson added this to the 1.5 milestone Jun 23, 2020
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants