Skip to content

Conversation

@goggle
Copy link
Contributor

@goggle goggle commented Oct 28, 2019

This PR addresses another issue from JuliaLang/LinearAlgebra.jl#485.

Consider this code:

using LinearAlgebra
using SparseArrays
A = sparse([1, 2], [1, 2], Float64[1.0, 1.0])
F = lu(A)

Prior to this PR, the LU factorization looked like this:

julia> F
UMFPACK LU Factorization of a (2, 2) sparse matrix
Ptr{Nothing} @0x0000557483015a10

Now it is shown in the same manner as the LU factorization of a dense matrix:

julia> F
SuiteSparse.UMFPACK.UmfpackLU{Float64,Int64}
L factor:
2×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
  [1, 1]  =  1.0
  [2, 2]  =  1.0
U factor:
2×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
  [1, 1]  =  1.0
  [2, 2]  =  1.0

@ViralBShah
Copy link
Member

Even the dense LU seems to show too much in my opinion. I wonder if we should show less output for the dense LU.

@ViralBShah ViralBShah added sparse Sparse arrays linear algebra Linear algebra labels Oct 29, 2019
@goggle
Copy link
Contributor Author

goggle commented Oct 29, 2019

I don't see any downsides in showing some detailed information about a performed matrix factorization. It's much more visually appealing than just showing a type (or something similar) in my opinion. And it only affects REPL sessions, where the user usually wants to have some interaction. Furthermore it can easily be prevented by adding a ; at the end of the line.

@ViralBShah
Copy link
Member

I agree with that - but this scrolls to two screenfuls! My preference would be to show fewer entries and restrict the printing to be about half a screenful.

@goggle
Copy link
Contributor Author

goggle commented Oct 29, 2019

To achieve this, we would need to crop the displayed matrices in a more aggressive manner, e.g. restrict the number of shown entries to 10. I would suggest to address this as a follow-up of JuliaLang/LinearAlgebra.jl#485, because it's not only about LU, but also all the other factorization types.

@andreasnoack
Copy link
Member

I think we should definitely get rid of the Ptr part of the printing. However, I'm generally not a fan of printing entries from a sparse array. I don't think it's useful information for sparse matrices so I'd rather provide some information about sparsity.

@ViralBShah
Copy link
Member

On better printing of sparse matrices, there's also: #30587

@goggle
Copy link
Contributor Author

goggle commented Oct 31, 2019

@ViralBShah Thanks, this is definitely interesting!

@dkarrasch
Copy link
Member

This PR is somewhat independent from the sparse printing, isn't it? Whatever is going on there, will be simply pasted in here, IIUC. Thus, ignoring that aspect, is this PR then acceptable? I'd say it's very much in line with what we have for other factorizations.

@andreasnoack
Copy link
Member

Given the development in the sparse printing I also think this is fine as it is.

@andreasnoack andreasnoack merged commit 71c4d9b into JuliaLang:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

linear algebra Linear algebra sparse Sparse arrays

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants