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

One-line show for SparseVector #427

Merged
merged 22 commits into from
Oct 11, 2023
Merged

One-line show for SparseVector #427

merged 22 commits into from
Oct 11, 2023

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 24, 2023

Fix #400 by implementing the suggestion in #400 (comment).

After this,

julia> S = sparsevec([1,4], [2,3])
4-element SparseVector{Int64, Int64} with 2 stored entries:
  [1]  =  2
  [4]  =  3

julia> show(S)
sparsevec([1, 4], [2, 3], 4)
julia> [S,S]
2-element Vector{SparseVector{Int64, Int64}}:
 sparsevec([1, 4], [2, 3], 4)
 sparsevec([1, 4], [2, 3], 4)

The displayed form is round-trippable as well.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #427 (9e68b7e) into main (e86b148) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   85.40%   85.43%   +0.03%     
==========================================
  Files          13       13              
  Lines        8734     8733       -1     
==========================================
+ Hits         7459     7461       +2     
+ Misses       1275     1272       -3     
Files Coverage Δ
src/sparsevector.jl 95.64% <100.00%> (+0.21%) ⬆️

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

@matbesancon
Copy link
Contributor

@jishnub fantastic! Some docstrings need fixing now that the printing changed: https://github.com/JuliaSparse/SparseArrays.jl/actions/runs/5985741971/job/16238147916?pr=427

Also, we should probably add some tests beyond the undef one you changed

@jishnub
Copy link
Contributor Author

jishnub commented Sep 29, 2023

Updated, and added tests. I think this works now. This doesn't change the display for arrays with zero elements:

julia> S = sparsevec(Int64[], Int64[], 3)
3-element SparseVector{Int64, Int64} with 0 stored entries

julia> [S]
1-element Vector{SparseVector{Int64, Int64}}:
 3-element SparseVector{Int64, Int64} with 0 stored entries

julia> show(S)
sparsevec(Int64[], Int64[], 3)

The difference arises as the display of S has only one line even in "text/plain", which makes [S] use it directly instead of using show(S) as in non-empty cases. I'm unsure what's the best way to address this, so I've left this unchanged for now (and this doesn't matter for the original issue)

docs/Project.toml Outdated Show resolved Hide resolved
@matbesancon
Copy link
Contributor

The difference arises as the display of S has only one line even in "text/plain", which makes [S] use it directly instead of using show(S) as in non-empty cases. I'm unsure what's the best way to address this, so I've left this unchanged for now (and this doesn't matter for the original issue)

@jishnub I believe it would make sense to also cover this case but this can be done in a separate issue, can you create one?

@matbesancon
Copy link
Contributor

In the meantime, we can merge this, thanks a lot!

@matbesancon matbesancon merged commit d884072 into main Oct 11, 2023
8 checks passed
@matbesancon matbesancon deleted the jishnub/sparsevecshow branch October 11, 2023 07:46
@dkarrasch
Copy link
Member

@matbesancon Please be careful about the merge option. This should have better been squashed and merged. For next time. 😉

@matbesancon
Copy link
Contributor

Oh my bad! I usually do, it slipped here

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.

Displayed form for vectors of sparse vectors may be improved
4 participants