Diagonal(::AbstractMatrix) behavior differs from documentation in certain cases

[KlausC]

  | | |_| | | | (_| |  |  Version 1.1.0-DEV.10 (2018-08-11)
 _/ |\__'_|_|_|\__'_|  |  sparsearrays-triangular-operations/72f777ee96 (fork: 8 commits, 0 days)
|__/                   |

Whereas docu states:

help?> Diagonal
  Diagonal(A::AbstractMatrix)
  Construct a matrix from the diagonal of A.

When applied to a subtype of AbstractMatrix we obtain the following results:

julia> A = UpperTriangular(sparse([1.0 0;20 0]))
2×2 UpperTriangular{Float64,SparseMatrixCSC{Float64,Int64}}:
 1.0  0.0
  ⋅   0.0

julia> Diagonal(A)
2×2 Diagonal{Float64,SparseVector{Float64,Int64}}:
 1.0   ⋅ 
  ⋅   0.0

julia> A[1,2] = 1
1

julia> Diagonal(A)
ERROR: ArgumentError: matrix cannot be represented as Diagonal
Stacktrace:
 [1] Diagonal(::UpperTriangular{Float64,SparseMatrixCSC{Float64,Int64}}) at /home/julia/julia/usr/share/julia/stdlib/v1.1/LinearAlgebra/src/special.jl:48
 [2] top-level scope at none:0

That means, in this case the Diagonal is not constructed from the diagonal of A, but an error is thrown, if isdiag(A) == false.
The same behavior applies for some other subtypes of AbstractMatrix, and not for others.
IMO the check for diagonality should be removed from the constructors, and in all cases
Diagonal(A::AbstractMatrix) = Diagonal(diag(A)) should be true.
Alternatively the docu string should warn the user about the error check in some and which cases.

[chriscoey]

IMO the check for diagonality should be removed from the constructors

Makes sense. Can this be fixed in 1.x?

[KlausC]

I think changing the doc is not a good solution, because that would complicate a simple situation and manifest the inconsistent behaviour.
Why should Diagonal(ones(2,2)) be allowed, but not Diagonal(UpperTriangular(ones(2,2))? Logically both are identical.

Looking at Bidiagonal and Tridiagonal gives a similar picture:

julia> Bidiagonal(UpperTriangular(ones(3,3)))
ERROR: ArgumentError: matrix cannot be represented as Bidiagonal
Stacktrace:
 [1] Bidiagonal(::UpperTriangular{Float64,Array{Float64,2}}) at /home/julia/julia/usr/share/julia/stdlib/v1.1/LinearAlgebra/src/special.jl:51
 [2] top-level scope at none:0

julia> Bidiagonal(UpperTriangular(ones(3,3)), :U)
3×3 Bidiagonal{Float64,Array{Float64,1}}:
 1.0  1.0   ⋅ 
  ⋅   1.0  1.0
  ⋅    ⋅   1.0
julia> Tridiagonal(ones(3,3))
3×3 Tridiagonal{Float64,Array{Float64,1}}:
 1.0  1.0   ⋅ 
 1.0  1.0  1.0
  ⋅   1.0  1.0

julia> Tridiagonal(UpperTriangular(ones(3,3)))
ERROR: ArgumentError: matrix cannot be represented as Tridiagonal
Stacktrace:
 [1] Tridiagonal(::UpperTriangular{Float64,Array{Float64,2}}) at /home/julia/julia/usr/share/julia/stdlib/v1.1/LinearAlgebra/src/special.jl:56
 [2] top-level scope at none:0

So the constructors for *Diagonal with one argument are especially critical, when the input argument is a 'special matrix', in case they require the input data to have the expected shape in advance. But if the input is another AbstractMatrix, there is no such check, but the unexpected data are simply ignored.

For other constructors, i.e. Upper/LowerTriangular, Symmetric there is also no input check for good reasons. The same is for Bidiagonal(arg, :U).

To describe the current situation the documentation for each *Diagonal would be something like:

If the constructor has exactly one argument, which isa Diagonal, Bidiagonal, Tridiagonal,
SymTridiagonal, or AbstractTriangular it acts as a conversion, that means if the shape of the
input doesn't fit the target type, an ArgumentError is thrown.
Otherwise the constructor is considered a partial copy, that means data outside the shape of
the target type are silently ignored.

I think, that is too complex to remember. It would also require extra effort, if you wanted to ignore out-of-shape data in your use case.
@StefanKarpinski, @andreasnoack

[bsxfan]

I would like to revive this issue. My reasons, similar to the discussion above, are given in a discourse post.