Skip to content

Use Braille to display adjoint and transposed sparse matrices #36177

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 6, 2021
Merged

Use Braille to display adjoint and transposed sparse matrices #36177

merged 6 commits into from
Jul 6, 2021

Conversation

@goggle
Copy link
Contributor

@goggle goggle commented Jun 7, 2020 •
edited
Loading

This PR fixes #34268.
Adjoint and transposed sparse matrices are now printed with Braille, similar to regular sparse matrices.

For some reason, merging the two identical Base.show methods into a method

function Base.show(io::IOContext, S::AbstractSparseMatrixCSCInclAdjointAndTranspose)

by using our introduced const type, results in weird error, which arises when trying to print a sparse matrix:

ERROR: MethodError: no method matching display(::SparseMatrixCSC{Float64,Int64})
Closest candidates are:
  display(::Any) at multimedia.jl:322
  display(::AbstractDisplay, ::AbstractString, ::Any) at multimedia.jl:214
  display(::AbstractString, ::Any) at multimedia.jl:215
  ...
Stacktrace:
 [1] display(::Any) at ./multimedia.jl:333
 [2] #invokelatest#1 at ./essentials.jl:710 [inlined]
 [3] invokelatest at ./essentials.jl:709 [inlined]
 [4] print_response(::IO, ::Any, ::Bool, ::Bool, ::Any) at /home/alex/Projects/github.com/julia/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:238
 [5] print_response(::REPL.AbstractREPL, ::Any, ::Bool, ::Bool) at /home/alex/Projects/github.com/julia/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:223
 [6] (::REPL.var"#do_respond#54"{Bool,Bool,Atom.var"#250#251",REPL.LineEditREPL,REPL.LineEdit.Prompt})(::Any, ::Any, ::Any) at /home/alex/Projects/github.com/julia/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:822
 [7] #invokelatest#1 at ./essentials.jl:710 [inlined]
 [8] invokelatest at ./essentials.jl:709 [inlined]
 [9] run_interface(::REPL.Terminals.TextTerminal, ::REPL.LineEdit.ModalInterface, ::REPL.LineEdit.MIState) at /home/alex/Projects/github.com/julia/usr/share/julia/stdlib/v1.6/REPL/src/LineEdit.jl:2354
 [10] run_frontend(::REPL.LineEditREPL, ::REPL.REPLBackendRef) at /home/alex/Projects/github.com/julia/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:1143
 [11] (::REPL.var"#38#42"{REPL.LineEditREPL,REPL.REPLBackendRef})() at ./task.jl:358

It took me so much time to figure that out, and I still don't know why this happened...
So that's the reason why I have that identical (except from the signature) Base.show method twice.

@ViralBShah ViralBShah added sparse Sparse arrays display and printing Aesthetics and correctness of printed representations of objects. labels Jun 8, 2020
StefanKarpinski
StefanKarpinski reviewed Jun 8, 2020
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl Outdated

const brailleBlocks = UInt16['⠁', '⠂', '⠄', '⡀', '⠈', '⠐', '⠠', '⢀']
function _show_with_braille_patterns(io::IOContext, S::AbstractSparseMatrixCSC)
function _show_with_braille_patterns(io::IOContext, S::AbstractSparseMatrixCSCInclAdjointAndTranspose)
Copy link
Member

@StefanKarpinski StefanKarpinski Jun 8, 2020

Choose a reason for hiding this comment

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

This linear algebra aliases are getting kind of ridiculous. Not a critique, just a humorous observation.

Copy link
Contributor Author

@goggle goggle Jun 8, 2020

Choose a reason for hiding this comment

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

Yes, I'm aware of that, I also don't like it.
If anybody has a better solution, feel free to suggest something.

Copy link
Member

@ViralBShah ViralBShah Jun 8, 2020

Choose a reason for hiding this comment

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

Not to be tackled in this PR!

goggle
goggle commented Jun 8, 2020
View reviewed changes
stdlib/SparseArrays/src/sparsematrix.jl Outdated
_show_with_braille_patterns(io, S)
end

function Base.show(io::IOContext, S::Union{Adjoint{<:Any,<:AbstractSparseMatrixCSC},Transpose{<:Any,<:AbstractSparseMatrixCSC}})
Copy link
Contributor Author

@goggle goggle Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is the method which I'm unable to replace with

function Base.show(io::IOContext, S::AbstractSparseMatrixCSCInclAdjointAndTranspose)

See the error in the description of the PR.

@stevengj
Copy link
Member

stevengj commented Feb 19, 2021

Any update on this?

@goggle goggle force-pushed the 34268-show-on-sparse-adjoints branch from 08e0504 to 61667d1 Compare February 25, 2021 03:52
@goggle
Copy link
Contributor Author

goggle commented Feb 25, 2021

I have done a rebase to resolve the conflicts.

@ViralBShah ViralBShah added the backport 1.6 Change should be backported to release-1.6 label Feb 25, 2021
@ViralBShah
Copy link
Member

ViralBShah commented Apr 28, 2021

Would be nice to rebase to master and see if we can get this ready for merging.

@simeonschaub simeonschaub force-pushed the 34268-show-on-sparse-adjoints branch from 2b909fc to ee35f7d Compare July 6, 2021 11:54
@simeonschaub simeonschaub added forgetmenot and removed backport 1.6 Change should be backported to release-1.6 labels Jul 6, 2021
@simeonschaub simeonschaub requested a review from ViralBShah July 6, 2021 13:35
@ViralBShah
Copy link
Member

ViralBShah commented Jul 6, 2021 •
edited
Loading

I haven't deeply reviewed the code, but broadly speaking, it looks good to me. Those really long type names make me cringe just a little bit - but there's nothing we can do about all that right now. :-)

@simeonschaub
Copy link
Member

simeonschaub commented Jul 6, 2021

Yeah, definitely agree. Will merge later today if there are no more comments.

@simeonschaub simeonschaub merged commit b9f2acc into JuliaLang:master Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@ViralBShah ViralBShah ViralBShah approved these changes

Assignees

No one assigned

Labels

display and printing Aesthetics and correctness of printed representations of objects. sparse Sparse arrays

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

show on sparse adjoints displays dense output

6 participants

@goggle @stevengj @ViralBShah @simeonschaub @StefanKarpinski @DilumAluthge