-
-
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
WIP: make transpose non-recursive #23424
Conversation
Also, this a prerequisite of JuliaLang/LinearAlgebra.jl#450 (which requires a non-recursively transposing |
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.
Nice! A general question: everything is @inline
d, is that necessary?
base/linalg/conjarray.jl
Outdated
@inline adjoint(a::ConjArray) = ConjAdjointArray(parent(a)) | ||
|
||
""" | ||
AdjointArray(array) |
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.
indent
base/linalg/conjarray.jl
Outdated
0+0im 1+1im | ||
``` | ||
""" | ||
struct AdjointArray{T,N,A<:AbstractArray} <: AbstractArray{T,N} |
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.
Perhaps struct AdjointArray{T,N,A<:AbstractArray{T,N}} <: AbstractArray{T,N}
or perhaps the element type of A
can be different from T
?
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.
N
stays the same but the eltype can change via the mapping through adjoint
. I can do struct AdjointArray{T,N,A<:AbstractArray{<:Any,N}} <: AbstractArray{T,N}
.
base/linalg/conjarray.jl
Outdated
|
||
@inline AdjointArray(a::AbstractArray{T,N}) where {T,N} = AdjointArray{adjoint_type(T),N,typeof(a)}(a) | ||
|
||
const AdjointVector{T,V<:AbstractVector} = AdjointArray{T,1,V} |
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.
const AdjointVector{T,V<:AbstractVector{T}}
unless eltype(V)
sometimes is not T
?
base/linalg/conjarray.jl
Outdated
const AdjointVector{T,V<:AbstractVector} = AdjointArray{T,1,V} | ||
@inline AdjointVector(v::AbstractVector{T}) where {T} = AdjointArray{adjoint_type(T),1,typeof(v)}(v) | ||
|
||
const AdjointMatrix{T,M<:AbstractMatrix} = AdjointArray{T,2,M} |
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.
const AdjointMatrix{T,M<:AbstractMatrix{T}}
base/linalg/conjarray.jl
Outdated
const AdjointMatrix{T,M<:AbstractMatrix} = AdjointArray{T,2,M} | ||
@inline AdjointMatrix(m::AbstractMatrix{T}) where {T} = AdjointArray{adjoint_type(T),2,typeof(m)}(m) | ||
|
||
# This type can cause the element type to change under conjugation - e.g. an array of complex arrays. |
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.
Guess this answers my own question above then.
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.
👍
base/linalg/conjarray.jl
Outdated
@inline conj(a::AdjointArray) = ConjAdjointArray(parent(a)) | ||
|
||
""" | ||
ConjAdjointArray(array) |
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.
indent
base/linalg/generic.jl
Outdated
@@ -929,7 +929,7 @@ issymmetric(x::Number) = x == x | |||
""" | |||
ishermitian(A) -> Bool | |||
|
|||
Test whether a matrix is Hermitian. | |||
Test whether a matrix is Hermitian (such that `A = adjoint(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.
==
base/linalg/rowvector.jl
Outdated
@@ -13,44 +13,37 @@ vector can be multiplied by a matrix on its right (such that `v.' * A = (A.' * v | |||
differs from a `1×n`-sized matrix by the facts that its transpose returns a vector and the | |||
inner product `v1.' * v2` returns a scalar, but will otherwise behave similarly. | |||
""" | |||
struct RowVector{T,V<:AbstractVector} <: AbstractMatrix{T} | |||
struct RowVector{T,V<:AbstractVector{T}} <: AbstractMatrix{T} |
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.
👍
base/linalg/conjarray.jl
Outdated
0+0im 1-1im | ||
``` | ||
""" | ||
struct ConjAdjointArray{T,N,A<:AbstractArray} <: AbstractArray{T,N} |
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.
Can this be ConjAdjointArray{T,N,A<:AbstractArray{T,N}} <: AbstractArray{T,N}
Great that you are working on this. A few comments after a first read
|
@andreasnoack Yes, I think a I think you are right regarding |
If we're going to have some kinds of deferred computation views of arrays in Base, why not do the full blown thing? It seems somewhat simpler than having a bunch of special cases and then having the general version in a package. |
Feel free to steal wildly from https://github.com/JuliaArrays/MappedArrays.jl. Note there are two types, differing in terms of whether you can or cannot define |
AFAICT you almost never need manual |
Right - this is wonderful Tim :) I haven't had the chance to try it out with large |
11ef34b
to
aef7238
Compare
I tend to agree... but I'm not sure what the community feels, or the best way forward. I mean - we need deferred computation of |
@timholy MappedArrays.jl looks cool. I took the ideas and merged the two classes into a single If I get it working I'll open a PR - any design opinions? |
Yes might as well bring in the general MappedArray here --- looks like a small amount of code. |
aef7238
to
fcf5201
Compare
As a side note when I see the name I don't think conjugate adjoint is a defined term, and therefore, no confusion could arise about its meaning, but I thought it was worth bringing up the above, since interpreting adjoint as |
8f699e7
to
dfee60c
Compare
7b0de64
to
7b6ca5d
Compare
6c60c1e
to
a52023a
Compare
FYI when I get tests passing, I'd like to merge this (not there yet, still bugs). Any last comments on e.g. |
I much appreciate the effort to move JuliaLang/LinearAlgebra.jl#57 and JuliaLang/LinearAlgebra.jl#408 forward that this pull request represents, and am on board with most of the associated plan. I would much like to also be on board with the terminology choices and downstream impact that this pull request advances, and so regularly try to convince myself that those choices yield the best set of tradeoffs available. Each time I try to convince myself of that position, I run aground on the same set of misgivings. One of those misgivings is the substantial implementation complexity this choice forces upon To illustrate, consider This problem worsens exponentially with the number of arguments. For example, without conflation This side effect seems worth considering before moving forward with this change. Best! |
I think that's more of a comment about the current design of I've sketched out a way this can work in a BandedMatrices.jl branch: https://github.com/JuliaMatrices/BandedMatrices.jl/blob/feat-bandslice/src/generic/interface.jl It dispatches by the trait type, not the matrix type. So for example if |
@dlfivefifty, can you expand? I do not follow how traits would address the problem of needing to handle an additional wrapper type with different semantics. Best! |
If it’s BLAS, knowing the stride and pointer is sufficient. So the type just needs to overload those. The trait helps determine if the BLAS interface is implemented. |
@dlfivefifty, these considerations are not confined to BLAS operations over dense arrays of BLAS-compatible scalar types, and in any case I am not certain I follow? Additionally, a complete redesign of |
I agree with @dlfivefifty . Pure julia fallback methods don't need to know whether a matrix is bare or wrapped, if only |
Perhaps I see the difference in perspective 🙂: The responses above seem to assume that operations over transpose-wrapped and array-flip-wrapped objects can always share an underlying implementation. This assumption holds in the most common and familiar cases, for example in operations that map to BLAS calls, where the semantics of transpose and array-flip coincide. Under that assumption, agreed, the additional complexity required to handle both transpose and array-flip in Even in such a system, however, handling both transpose and array-flip would still require an appreciable level of additional complexity. And lovely as such a system would be, the existing system neither is nor is likely to evolve into such a system by feature freeze. And in general, operations over transpose-wrapped and array-flip-wrapped objects cannot always share an (efficient) underlying implementation. The difference in behavior between transpose-wrapped and array-flip-wrapped objects --- that the former is recursive, whereas the latter is not -- necessarily propagates all the way down to the elementary operations that constitute a given higher level operation, and so cannot always be efficiently handled at shallow levels in call chains.
That statement is, to say the least, a minor trivialization of the importance and difficulty of writing efficient kernels for even simple operations that do not immediately map to e.g. BLAS calls 😄. Most operations in |
The most important aspect to take into account is loop order in combination with proper blocking no? Which could be checked by looking at the strides rather than at what particular type of wrapper it is (if the data is not strided than it's probably impossible to decide on an optimal strategy). I've been working on a package (https://github.com/Jutho/Strided.jl) to deal with strided data with arbitrary strides (such that Still different from linear algebra operations, but I would think similar constructions are possible. But indeed, not likely in the current structure of |
@Sacha0 Yes, important points to consider. My thoughts on the way forward:
Perhaps I'm being overly optimisitc, but with the right traits and so-on I thought we might get to a decent internal system in the future (not v1.0). |
@Jutho, this work sounds super exciting! I much look forward to checking this package out! 😃
Remember that: (1) |
These can each be handled by a trait, This would allow user defined matrices to also benefit from optimal implementation. |
These responses implicitly affirm that gracefully handling this approach's additional complexity would require substantial additions to and/or structural overhaul of |
68acd8e
to
3232750
Compare
* RowVector no longer transposes elements * MappedArray used to propagate conj, transpose, adjoint, etc
3232750
to
7da9eaf
Compare
Since we aren't doing non-recursive transpose, I'll close this PR. Data users that have been following this, please use the new, friendlier |
Step 2 of JuliaLang/LinearAlgebra.jl#408 "Taking matrix transposes seriously". Currently WIP - I hope to at least add some more unit tests and have the design questions below answered.
This PR continues to have a recursive
adjoint
for doing linear algebra (on possibly nested arrays, or other user defined structures) but removes the recursion fortranspose
. In effect,transpose
is being "gifted" to data applications as a simple way to rearrange data in containers (likepermutedims
) - for example, we can now do["abc", "def"].'
. I also have corrected any non-recursiveadjoint
behavior as I found it.One consequence which seems odd at first but makes more sense when you think about it is that some linear algebra applications will need to use
conj(m')
instead ofm.'
. This doesn't seem so bad since this is actually how legitimate linear algebra uses of.'
arises anyway.The design for
RowVector
extends on what was already there, adding recursiveAdjointArray
wrappers just like the existing lazyConjArray
wrapper. The idea is to do the same fortranspose(::AbstractMatrix)
andadjoint(::AbstractMatrix)
soon, so that transpose there is lazy and we can removeAc_mul_B
and so-on. Future work will also involve moving functions into appropriate files, but since work is ongoing I probably would prefer to do that in a separate PR.Two design questions:
conjadjoint(x)
function? It seems to come up occassionally, and would be easy to elide in appropriate cases, e.g. viaconjadjoint(x::Number) = x
. (Going the opposite direction, we could removeConjAdjointArray
...)Symmetric
andissymmetric
be recursive? Their names make it seem a bit like they should go withtranspose
and work on "data", however most common usage is for linear algebra and it is a part ofLinAlg
. The two choices areissymmetric(m) = m == m.'
orissymmetric(m) = m == conj(m')
.CC: @StefanKarpinski @stevengj @andreasnoack @Sacha0