Skip to content

Compact printing for Adjoint vectors, and Diagonal #40722

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 6 commits into from
Aug 6, 2024
Merged

Compact printing for Adjoint vectors, and Diagonal #40722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ed6764
show Diagonal as constructor
mcabbott May 5, 2021
1b512a9
show adjoint vectors not as matrices
mcabbott May 5, 2021
5119028
Merge branch 'master' into diagshow
jishnub Jul 19, 2024
373bd8e
Fix whitespace
jishnub Jul 20, 2024
1a3cff0
fix one doctest
mcabbott Jul 29, 2024
34376b5
Merge branch 'master' into diagshow
mcabbott Aug 4, 2024
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
2 changes: 1 addition & 1 deletion base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1921,7 +1921,7 @@ julia> vcat(range(1, 2, length=3)) # collects lazy ranges
2.0

julia> two = ([10, 20, 30]', Float64[4 5 6; 7 8 9]) # row vector and a matrix
([10 20 30], [4.0 5.0 6.0; 7.0 8.0 9.0])
(adjoint([10, 20, 30]), [4.0 5.0 6.0; 7.0 8.0 9.0])

julia> vcat(two...)
3×3 Matrix{Float64}:
Expand Down
10 changes: 10 additions & 0 deletions stdlib/LinearAlgebra/src/adjtrans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,16 @@ function Base.showarg(io::IO, v::Transpose, toplevel)
toplevel && print(io, " with eltype ", eltype(v))
return nothing
end
function Base.show(io::IO, v::Adjoint{<:Real, <:AbstractVector})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this omits vectors of complex numbers... because I thought that printing 0 + 3im when the element is in fact 0 - 3im here is worse than failing to note the container type:

julia> (transpose([1,2,3im]), adjoint([1,2,3im]))
(transpose(Complex{Int64}[1 + 0im, 2 + 0im, 0 + 3im]), Complex{Int64}[1 + 0im 2 + 0im 0 - 3im])

It also doesn't change how e.g. an array of matrices is printed, for the same reason -- printing [1 2; 3 4] which isn't == the element seems confusing:

julia> ([[1 2; 3 4], [5 6; 7 8]]',)
(Adjoint{Int64, Matrix{Int64}}[[1 3; 2 4] [5 7; 6 8]],)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the choices here

Copy link
Member

Choose a reason for hiding this comment

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

Should we add tests for the complex case, making it explicit that this was a design choice and not an oversight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the restrictions to v::Adjoint{<:Real, <:AbstractVector} and v::Transpose{<:Number, <:AbstractVector} are fairly obviously deliberate.

For the complex case, it would not be crazy to print adjoint(conj([1 + 0im, 2 + 0im, 0 - 3im])) as this also resolves to the right object (since conj is eager). Maybe I'd rather not have explicit tests blocking that.

print(io, "adjoint(")
show(io, parent(v))
print(io, ")")
end
function Base.show(io::IO, v::Transpose{<:Number, <:AbstractVector})
print(io, "transpose(")
show(io, parent(v))
print(io, ")")
end

# some aliases for internal convenience use
const AdjOrTrans{T,S} = Union{Adjoint{T,S},Transpose{T,S}} where {T,S}
Expand Down
5 changes: 5 additions & 0 deletions stdlib/LinearAlgebra/src/diagonal.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,11 @@ end
function Base.replace_in_print_matrix(A::Diagonal,i::Integer,j::Integer,s::AbstractString)
i==j ? s : Base.replace_with_centered_mark(s)
end
function Base.show(io::IO, A::Diagonal)
Copy link
Member

Choose a reason for hiding this comment

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

Should we add similar methods to the other banded matrix types?

Copy link
Contributor Author

@mcabbott mcabbott Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could do, but maybe not this PR?

For the more complicated ones, there are choices like whether to print SymTridiagonal([1,2,3], [4,5]) or rather SymTridiagonal([1 4 0; 4 2 5; 0 5 3]).

In this PR, only Diagonal is the only upper-case constructor printed.

Note also that not all Adjoint or Transpose objects are affected, only the simplest ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Xref #55347 for Bidiagonal.

print(io, "Diagonal(")
show(io, A.diag)
print(io, ")")
end

parent(D::Diagonal) = D.diag

Expand Down
5 changes: 5 additions & 0 deletions stdlib/LinearAlgebra/test/adjtrans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,11 @@ end
@test String(take!(io)) == "transpose(::Matrix{Float64})"
end

@testset "show" begin
@test repr(adjoint([1,2,3])) == "adjoint([1, 2, 3])"
@test repr(transpose([1f0,2f0])) == "transpose(Float32[1.0, 2.0])"
end

@testset "strided transposes" begin
for t in (Adjoint, Transpose)
@test strides(t(rand(3))) == (3, 1)
Expand Down
5 changes: 5 additions & 0 deletions stdlib/LinearAlgebra/test/diagonal.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,11 @@ Base.size(::SMatrix1) = (1, 1)
@test C isa Matrix{SMatrix1{String}}
end

@testset "show" begin
@test repr(Diagonal([1,2])) == "Diagonal([1, 2])" # 2-arg show
@test contains(repr(MIME"text/plain"(), Diagonal([1,2])), "⋅ 2") # 3-arg show
end

@testset "copyto! with UniformScaling" begin
@testset "Fill" begin
for len in (4, InfiniteArrays.Infinity())
Expand Down