Skip to content

un-revert "Simplify some views of Adjoint matrices" #1122

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 4, 2024
Merged

un-revert "Simplify some views of Adjoint matrices" #1122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/adjtrans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,20 @@ parent(A::AdjOrTrans) = A.parent
vec(v::TransposeAbsVec{<:Number}) = parent(v)
vec(v::AdjointAbsVec{<:Real}) = parent(v)

Base.reshape(v::TransposeAbsVec{<:Number}, ::Val{1}) = parent(v)
Copy link
Member

Choose a reason for hiding this comment

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

Should reshape(v, ::Colon) and such behave identically? Or is the Val(1) reshaping used in the view below? E.g.:

julia> v = 1:4
1:4

julia> reshape(v', :)
4-element reshape(adjoint(::UnitRange{Int64}), 4) with eltype Int64:
 1
 2
 3
 4

julia> reshape(v', Val(1))
1:4

It might be a good idea to have these all return a range, but that may be a separate PR.

Copy link
Collaborator Author

@mcabbott mcabbott Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yea this PR is only about view. And some views call this (internal?) method with Val.

The "supported" way to do that reshape is:

julia> vec(v')
1:4

The default definition is vec(a::AbstractArray) = reshape(a,length(a)), and at present reshape(v', 4) also has no shortcut.

Base.reshape(v::AdjointAbsVec{<:Real}, ::Val{1}) = parent(v)

# these make eachrow(A') produce simpler views
@inline Base.unsafe_view(A::Transpose{<:Number, <:AbstractMatrix}, i::Integer, j::AbstractArray) =
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps use the alias TransposeAbsMat like the TransposeAbsVec alias used in reshape?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IDK why it's written this way, it's possible I followed nearby code which writes e.g. Base.strides(A::Adjoint{<:Real, <:AbstractMatrix}) and does not seem to use TransposeAbsMat. Maybe I was concerned about order of definitions or something?

Base.unsafe_view(parent(A), j, i)
Copy link
Member

@jishnub jishnub Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These should probably call view on the parent, as the parent might have a specialized view implementation that avoids the SubArray wrapper.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we also do something similar for transposes of AbstractVectors, with one AbstractVector index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I initially said the wrong thing, because I've forgotten how this all works, but in fact that case is already handled (and tested). It's why the reshape(v, ::Val{1}) method is defined above.

I'm hesitant to start editing as I do not have the details fresh in my head, and do not wish to introduce fresh mistakes. The pervious PR reverted mine with no changes, this PR un-does the reversion.

@inline Base.unsafe_view(A::Transpose{<:Number, <:AbstractMatrix}, i::AbstractArray, j::Integer) =
Base.unsafe_view(parent(A), j, i)

@inline Base.unsafe_view(A::Adjoint{<:Real, <:AbstractMatrix}, i::Integer, j::AbstractArray) =
Base.unsafe_view(parent(A), j, i)
@inline Base.unsafe_view(A::Adjoint{<:Real, <:AbstractMatrix}, i::AbstractArray, j::Integer) =
Base.unsafe_view(parent(A), j, i)

### concatenation
# preserve Adjoint/Transpose wrapper around vectors
# to retain the associated semantics post-concatenation
Expand Down
29 changes: 29 additions & 0 deletions test/adjtrans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,35 @@ end
@test vec(adjoint(mvec))[1] == adjoint(mvec[1])
end

@testset "Adjoint and Transpose view methods" begin
intvec, intmat = [1, 2], [1 2 3; 4 5 6]
# overload of reshape(v, Val(1)) simplifies views of row vectors:
@test view(adjoint(intvec), 1:2) isa SubArray{Int, 1, Vector{Int}}
@test view(transpose(intvec), 1:2) isa SubArray{Int, 1, Vector{Int}}
cvec = [1, 2im, 3, 4im]
@test view(transpose(cvec), 2:3) === view(cvec, 2:3)
@test view(adjoint(cvec), 2:3) == conj(view(cvec, 2:3))

# vector slices of transposed matrices are simplified:
@test view(adjoint(intmat), 1, :) isa SubArray{Int, 1, Matrix{Int}}
@test view(transpose(intmat), 1, :) isa SubArray{Int, 1, Matrix{Int}}
@test view(adjoint(intmat), 1, :) == permutedims(intmat)[1, :]
@test view(transpose(intmat), 1:1, :) == permutedims(intmat)[1:1, :] # not simplified
@test view(adjoint(intmat), :, 2) isa SubArray{Int, 1, Matrix{Int}}
@test view(transpose(intmat), :, 2) isa SubArray{Int, 1, Matrix{Int}}
@test view(adjoint(intmat), :, 2) == permutedims(intmat)[:, 2]
@test view(transpose(intmat), :, 2:2) == permutedims(intmat)[:, 2:2] # not simplified
cmat = [1 2im 3; 4im 5 6im]
@test view(transpose(cmat), 1, :) isa SubArray{Complex{Int}, 1, Matrix{Complex{Int}}}
@test view(transpose(cmat), :, 2) == cmat[2, :]
@test view(adjoint(cmat), :, 2) == conj(cmat[2, :]) # not simplified

# bounds checks happen before this
@test_throws BoundsError view(adjoint(intvec), 0:3)
@test_throws BoundsError view(transpose(cvec), 0:3)
@test_throws BoundsError view(adjoint(intmat), :, 3)
end

@testset "horizontal concatenation of Adjoint/Transpose-wrapped vectors and Numbers" begin
# horizontal concatenation of Adjoint/Transpose-wrapped vectors and Numbers
# should preserve the Adjoint/Transpose-wrapper to preserve semantics downstream
Expand Down