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

Make transposes of StridedArrays strided #29135

Merged
merged 5 commits into from
May 4, 2020
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Sep 11, 2018

but only in cases where the transpose is actually stored in memory.

Copy link
Contributor

@dlfivefifty dlfivefifty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be at all beneficial to use reverse(strides(parent(A))) ?

@mbauman
Copy link
Member Author

mbauman commented Sep 14, 2018

Sure enough:

julia> t = transpose(rand(3,4));

julia> s1(A) = (stride(A.parent, 2), stride(A.parent, 1))
       s2(A) = reverse(strides(A.parent))
s2 (generic function with 1 method)

julia> @btime s1($t)
  7.056 ns (0 allocations: 0 bytes)
(3, 1)

julia> @btime s2($t)
  2.032 ns (0 allocations: 0 bytes)
(3, 1)

@mbauman
Copy link
Member Author

mbauman commented Sep 14, 2018

Oh wait, that won't quite do it because of transposed vectors… should have actually tested it more thoroughly locally.

@dlfivefifty
Copy link
Contributor

_adjoint_strides((a,)) = (a,1)
_adjoint_strides((a,b)) = (b,a)
strides(A::Adoint) = _adjoint_strides(strides(parent(A)))

@mbauman
Copy link
Member Author

mbauman commented Sep 14, 2018

The answer for strided vectors is neither reverse(strides(A)) nor (stride(A,1), 1) nor (length(A), stride(A, 1)). Given that all three of us got this optimization wrong at first glance, I'm heavily inclined to just ask the vector itself for its stride(A, 2) instead of trying to re-implement that computation ourselves as a micro-optimization.

(I believe the correct answer is s = stride(A, 1); return (length(A)*s, s), but given that you should only ever multiply this number by 0 the point is somewhat moot… although you might get yourself in trouble with some error-checking routines that assert the strides are non-overlapping).

@mbauman
Copy link
Member Author

mbauman commented Apr 1, 2020

Let's re-run CI and get this in.

@mbauman mbauman closed this Apr 1, 2020
@mbauman mbauman reopened this Apr 1, 2020
@mcabbott
Copy link
Contributor

mcabbott commented Apr 1, 2020

Perhaps related, pointer(transpose(ones(2,2)) gives an error right now (on Julia 1.4), should this also work?

@mbauman
Copy link
Member Author

mbauman commented Apr 1, 2020

Yes, I think we could add pointer(::Transpose) (and it make sense to do so in this PR) but I don't think we should add pointer(::Adjoint) as the latter elements aren't actually stored on disk.

@mcabbott
Copy link
Contributor

mcabbott commented Apr 1, 2020

Shouldn't it follow strides and work for an Adjoint of real numbers?

@mbauman
Copy link
Member Author

mbauman commented Apr 1, 2020

Ah, yes, I had forgotten I did that. That's perfect.

@mbauman mbauman force-pushed the mb/stridedtransposes branch from 5dc0bf3 to ce847fd Compare April 1, 2020 22:27
@@ -186,6 +186,16 @@ IndexStyle(::Type{<:AdjOrTransAbsMat}) = IndexCartesian()
convert(::Type{Adjoint{T,S}}, A::Adjoint) where {T,S} = Adjoint{T,S}(convert(S, A.parent))
convert(::Type{Transpose{T,S}}, A::Transpose) where {T,S} = Transpose{T,S}(convert(S, A.parent))

# Strides and pointer for transposed strided arrays — but only if the elements are actually stored in memory
Base.strides(A::Adjoint{<:Real, <:StridedVector}) = (stride(A.parent, 2), stride(A.parent, 1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not restrict to <: StridedVector. The strided array interface means other array types should be supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I fully agree, but this is intended as a stopgap and not a full solution.

@dlfivefifty
Copy link
Contributor

FYI In ArrayLayouts.jl I came up with the following way to deal with the complex adjoint pointer issue: introduce ConjPtr to represent that the pointer is to the conjugate of the element.

"""
    ConjPtr{T}

represents that the entry is the complex-conjugate of the pointed to entry.
"""
struct ConjPtr{T} 
    ptr::Ptr{T}
end

unsafe_convert(::Type{Ptr{T}}, A::Adjoint{<:Real}) where T<:Real = unsafe_convert(Ptr{T}, parent(A))
unsafe_convert(::Type{Ptr{T}}, A::Transpose) where T = unsafe_convert(Ptr{T}, parent(A))
# work-around issue with complex conjugation of pointer
unsafe_convert(::Type{Ptr{T}}, Ac::Adjoint{<:Complex}) where T<:Complex = unsafe_convert(ConjPtr{T}, parent(Ac))
unsafe_convert(::Type{ConjPtr{T}}, Ac::Adjoint{<:Complex}) where T<:Complex = unsafe_convert(Ptr{T}, parent(Ac))    
function unsafe_convert(::Type{ConjPtr{T}}, V::SubArray{T,2}) where {T,N,P}
    kr, jr = parentindices(V)
    unsafe_convert(Ptr{T}, view(parent(V)', jr, kr))
end

@mbauman mbauman force-pushed the mb/stridedtransposes branch from ce847fd to 7d1cf61 Compare April 28, 2020 02:22
@mbauman mbauman closed this Apr 28, 2020
@mbauman mbauman reopened this Apr 28, 2020
@mcabbott
Copy link
Contributor

Bump, for 1.5?

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Apr 30, 2020
@mbauman
Copy link
Member Author

mbauman commented May 4, 2020

So was this discussed on the triage call? I really don't think it should be all that controversial — the only controversial thing is how minimally I've scoped it in an effort to keep in uncontroversial.

@StefanKarpinski
Copy link
Member

Your call, @mbauman.

@mbauman
Copy link
Member Author

mbauman commented May 4, 2020

Let's do this then. Part of the reason it's scoped so narrowly is to ensure we have space to move towards traits or whatever the bigger solution turns out to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants