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.

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

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented May 5, 2021

This changes the compact printing to preserve more information -- an adjoint vector is not quite a matrix, and Diagonal wastes a lot of space:

julia> (Diagonal(1:4), [5,6,7]', transpose(8:10))  # before
([1 0 0 0; 0 2 0 0; 0 0 3 0; 0 0 0 4], [5 6 7], [8 9 10])

julia> (Diagonal(1:4), [5,6,7]', transpose(8:10))  # after
(Diagonal(1:4), adjoint([5, 6, 7]), transpose(8:10))

Would have been better to do at the same time as 1.6's other printing changes, I guess.

@dkarrasch dkarrasch added display and printing Aesthetics and correctness of printed representations of objects. linear algebra Linear algebra labels May 5, 2021
@@ -136,6 +136,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

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.

@jishnub
Copy link
Member

jishnub commented Jul 21, 2024

There are some doctest failures, but the other test failures are unrelated. If there are no objections, it would be good to fix and merge this.

@@ -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.

@mcabbott mcabbott requested a review from jishnub August 5, 2024 13:26
@jishnub jishnub merged commit 1b32fa6 into JuliaLang:master Aug 6, 2024
7 checks passed
@mcabbott mcabbott deleted the diagshow branch August 6, 2024 03:53
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
This changes the compact printing to preserve more information -- an
adjoint vector is not quite a matrix, and Diagonal wastes a lot of
space:
```julia
julia> (Diagonal(1:4), [5,6,7]', transpose(8:10))  # before
([1 0 0 0; 0 2 0 0; 0 0 3 0; 0 0 0 4], [5 6 7], [8 9 10])

julia> (Diagonal(1:4), [5,6,7]', transpose(8:10))  # after
(Diagonal(1:4), adjoint([5, 6, 7]), transpose(8:10))
```
Would have been better to do at the same time as 1.6's other printing
changes, I guess.

---------

Co-authored-by: Jishnu Bhattacharya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants