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

When printing matrices, replace zeros by dots #1459

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

This makes it easier to read many matrices, especially those which are relatively sparse.

Before:

julia> matrix(QQ, [42 0 0; 0 42 0; 0 0 42])
[42//1    0//1    0//1]
[ 0//1   42//1    0//1]
[ 0//1    0//1   42//1]

After:

julia> matrix(QQ, [42 0 0; 0 42 0; 0 0 42])
[42//1       .       .]
[    .   42//1       .]
[    .       .   42//1]

This is breaking in so far as it will require changes to a bunch of doctests in Oscar.jl and probably other packages.

I expect this will be somewhat controversial, but I wanted to at least put it out there. To me it is subjectively a major improvement but I realize others may have a quite different opinion. But I figured we could at least discuss it.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.67%. Comparing base (5170909) to head (cc58444).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
- Coverage   88.10%   86.67%   -1.43%     
==========================================
  Files         120      120              
  Lines       30045    30021      -24     
==========================================
- Hits        26470    26021     -449     
- Misses       3575     4000     +425     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Oct 4, 2023

Hm, I have a different opinion. I don't like the dots.

I don't like the superfluous // either, but using Nemo/Oscar, it looks much better and I don't see a need for dots:

julia> matrix(QQ, [42 0 0; 0 42 0; 0 0 42])
[42    0    0]
[ 0   42    0]
[ 0    0   42]

@fingolfin
Copy link
Member Author

For me the application are biggish matrices over finite fields where I can see the structure (e.g. block diagonal etc.) at a glance with the "." printing but not with integers, e.g. here an example over GF(11):

[1   8   2   0   0   0]
[7   1   8   0   8   0]
[0   1   1   0   0   0]
[0   0   0   1   2   3]
[0   0   0   9   8   0]
[0   0   0   0   7   8]

where I don't immediately see what's going on, but

[1   8   2   .   .   .]
[7   1   8   .   8   .]
[.   1   1   .   .   .]
[.   .   .   1   2   3]
[.   .   .   9   8   .]
[.   .   .   .   7   8]

makes it stand out.

@thofma
Copy link
Member

thofma commented Oct 4, 2023

I find

julia> zero_matrix(QQ, 2, 2)
[.   .]
[.   .]

rather obscure. To be honest, I am not too excited about this change.

Maybe we could hide it behind a preference?

@fingolfin
Copy link
Member Author

Having a preferences for this sounds useful, just like for the unicode printing -- and just like for that, we should then have a way to temporarily turn it off, and also turn it off (or on, for testing the feature itself) in test suites

This makes it easier to read many matrices, especially those which
are relatively sparse.

Before:

    julia> matrix(QQ, [42 0 0; 0 42 0; 0 0 42])
    [42//1    0//1    0//1]
    [ 0//1   42//1    0//1]
    [ 0//1    0//1   42//1]

After:

    julia> matrix(QQ, [42 0 0; 0 42 0; 0 0 42])
    [42//1       .       .]
    [    .   42//1       .]
    [    .       .   42//1]
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.

2 participants