Skip to content

Conversation

@jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 14, 2023

Fix #399

On main:

julia> S = sprand(Int8, 20, 20, 0.3);

julia> [S]
1-element Vector{SparseMatrixCSC{Int8, Int64}}:
 sparse([2, 3, 4, 8, 9, 13, 14, 16, 18, 2    8, 13, 16, 17, 19, 4, 6, 10, 18, 19], [1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 6, 7, 7, 7, 7, 8, 8, 8, 8, 8, 8, 8, 8, 9, 9, 10, 10, 10, 10, 11, 11, 11, 11, 11, 12, 12, 12, 12, 12, 12, 12, 13, 13, 13, 13, 13, 13, 13, 13, 13, 14, 14, 14, 15, 15, 15, 15, 15, 15, 16, 16, 16, 16, 16, 16, 17, 17, 17, 17, 17, 18, 18, 18, 18, 18, 19, 19, 19, 19, 19, 20, 20, 20, 20, 20], Int8[108, 6, -86, 17, 48, 51, 23, 88, 82, -24    -27, -3, 24, -56, 109, 73, 70, 101, -100, 46], 20, 20)

This PR

julia> [S]
1-element Vector{SparseMatrixCSC{Int8, Int64}}:
 sparse([2, 3, 4, 8, 9, 13, 14, 16, 18, 2    8, 13, 16, 17, 19, 4, 6, 10, 18, 19], [1, 1, 1, 1, 1, 1, 1, 1, 1, 2    19, 19, 19, 19, 19, 20, 20, 20, 20, 20], Int8[108, 6, -86, 17, 48, 51, 23, 88, 82, -24    -27, -3, 24, -56, 109, 73, 70, 101, -100, 46], 20, 20)

The main change is to use an AbstractVector for the column indices instead of printing them one by one to the display, which ensures that the IOContext is automatically respected in the same way as the row indices and the values.

Indexing into the AbstractVector is O(log n) as it uses searchsortedlast, but since this is used only in display, the performance should not matter much.

@jishnub jishnub changed the title Respect IOContext in printing column indices Respect IOContext while displaying a SparseMatrixCSC Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #423 (1b7e1b1) into main (3d1eda9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
+ Coverage   92.41%   92.42%   +0.01%     
==========================================
  Files          12       12              
  Lines        7669     7666       -3     
==========================================
- Hits         7087     7085       -2     
+ Misses        582      581       -1     
Files Changed Coverage Δ
src/sparsematrix.jl 95.68% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jishnub jishnub requested a review from rayegun August 17, 2023 06:04
@rayegun
Copy link
Member

rayegun commented Aug 23, 2023

Can we do this without introducing a new struct? I'm not sure I see the benefit of a new type for a single function.

@jishnub
Copy link
Contributor Author

jishnub commented Aug 23, 2023

What approach would you recommend? One may duplicate the code for printing an AbstractVector, but that seems to only increase redundancy. What's the issue with introducing a struct, especially one that doesn't feature in the core functions?

Copy link
Member

@rayegun rayegun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking SGTM. You're right this is probably the least overall code.

If you wanted to do it as an iterator that would work as well.

If you could add a docstring (what it's for and state that it's not part of the public interface) I'll merge.

@rayegun rayegun enabled auto-merge (squash) August 24, 2023 08:06
@rayegun rayegun merged commit 4e2d1e4 into main Aug 24, 2023
@rayegun rayegun deleted the jishnub/showcolinds branch August 24, 2023 12:11
matbesancon pushed a commit to matbesancon/SparseArrays.jl that referenced this pull request Sep 3, 2023
…#423)

* Respect IOContext in printing column indices

* Add docstring to ColumnIndices
dkarrasch pushed a commit that referenced this pull request Sep 25, 2023
* Respect IOContext in printing column indices

* Add docstring to ColumnIndices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

show doesn't print a compact representation of J

3 participants