Skip to content
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

Improve REPL printing of UmfpackLU #33705

Merged
merged 1 commit into from
Nov 20, 2019
Merged

Conversation

goggle
Copy link
Contributor

@goggle goggle commented Oct 28, 2019

This PR addresses another issue from #24588.

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.

@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 #24588, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants