Allocate less in limited sparse printing #22604
Conversation
tkelman
added
domain:display and printing
iterate and print in forwards order instead of saving to an array and reversing use searchsortedfirst to find which column to start in fix tests for 32 bit
tkelman
force-pushed
the
tk/less-allocation
branch
from
7e25b9f
to
c2bcd15
Compare
test/sparse/sparse.jl
Outdated
| @@ -1779,6 +1779,21 @@ end | |||
| show(ioc, MIME"text/plain"(), sparse(Int64[1, 1], Int64[1, 2], [1.0, 2.0])) | |||
| @test String(take!(io)) == "1×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:\n ⋮" | |||
|
|
|||
| # even number of rows | |||
| ioc = IOContext(IOContext(io, :displaysize => (8, 80)), :limit => true) | |||
There was a problem hiding this comment.
(Tangent: The docstrings for IOContext would benefit from more examples. Edit: Opened an intro issue (#22637). )
There was a problem hiding this comment.
very much so, that feature has been underdocumented ever since it was added
test/sparse/sparse.jl
Outdated
| @@ -1779,6 +1779,21 @@ end | |||
| show(ioc, MIME"text/plain"(), sparse(Int64[1, 1], Int64[1, 2], [1.0, 2.0])) | |||
| @test String(take!(io)) == "1×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:\n ⋮" | |||
|
|
|||
| # even number of rows | |||
| ioc = IOContext(IOContext(io, :displaysize => (8, 80)), :limit => true) | |||
There was a problem hiding this comment.
Could simplify the nested IOContext calls to ioc = IOContext(io, displaysize = (8, 80), limit = true) (assuming I understand the docstring correctly and my experiments at the REPL confirm what they appear to)?
test/sparse/sparse.jl
Outdated
| @@ -1779,6 +1779,21 @@ end | |||
| show(ioc, MIME"text/plain"(), sparse(Int64[1, 1], Int64[1, 2], [1.0, 2.0])) | |||
| @test String(take!(io)) == "1×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:\n ⋮" | |||
|
|
|||
| # even number of rows | |||
| ioc = IOContext(IOContext(io, :displaysize => (8, 80)), :limit => true) | |||
| show(ioc, MIME"text/plain"(), sparse(Int64[1,2,3,4], Int64[1,1,2,2], [1.0,2.0,3.0,4.0])) | |||
There was a problem hiding this comment.
(Another tangent: That methods of show accepting a stream aren't !-terminated seems inconsistent with the usual convention? Edit: Did not find anything during a brief issue search, so noted in the API consistency thread #20402 (comment).)
base/sparse/sparsematrix.jl
Outdated
| print(io, "\n \u22ee") | ||
| # find the column to start printing in for the last print_count elements | ||
| nextcol = searchsortedfirst(S.colptr, nnz(S) - print_count + 1) | ||
| for r = nnz(S) - print_count + 1 : S.colptr[nextcol] - 1 |
There was a problem hiding this comment.
Though not strictly necessary, parens would make the range much easier to parse?
|
Will merge tomorrow then if I don't hear otherwise |
test/sparse/sparse.jl
Outdated
| @@ -1807,7 +1807,7 @@ end | |||
| @test String(take!(io)) == string("6×3 SparseMatrixCSC{Float64,$Int} with 18 stored entries:\n [1, 1]", | |||
| " = 1.0\n [2, 1] = 1.0\n ⋮\n [5, 3] = 1.0\n [6, 3] = 1.0") | |||
|
|
|||
| ioc = IOContext(io, :displaysize => (9, 80)) | |||
| ioc = IOContext(io, displaysize = (9, 80)) | |||
There was a problem hiding this comment.
As of today, it seems to be faster and to allocate less to use the Pair version rather than keywords; so this commit was more for code-style rather than for reducing allocations right?
There was a problem hiding this comment.
the iocontext part was stylistic yes (see collapsed review at #22604 (comment)), but in test code would the difference be noticeable?
There was a problem hiding this comment.
OK thanks; I don't believe it would be noticeable in test code, but in 893a65b Jameson did exactly the opposite for other test files, apparently in a performance related PR -- but he may have done it in test files only for stylistic reasons too ;-)
Avoids unnecessary allocation and reversal of an array of outputs from the method in #22536. Add a few more test cases with dense columns to be sure this is consistent with the output from master. The new tests should pass with or without the rest of the change.
edit: mac travis hung while downloading something, log backed up to https://gist.github.com/6433751028a10ec3d17391cd57bf0c26 and restarted