skip show when denominator is one

[kalmarek]

following #42626 with this pull we have:

julia> -2//2
-1//1

julia> [-2//2,]
1-element Vector{Rational{Int64}}:
 -1

julia> rand(-6:6, 4,4).//2
4×4 Matrix{Rational{Int64}}:
  -3      1    3//2  1//2
 -3//2    3     2     1
   2      0    -2    1//2
   0    -3//2  -1    -2

[kalmarek]

Whom should I ping to get this going? @KristofferC maybe?

[oscardssmith]

Jeff thinks this is too probably fancy (and is fairly different from Matrix{Bool} since there it would apply to all of the entries).
The rest of triage doesn't care.

[kalmarek]

Jeff thinks this is too probably fancy (and is fairly different from Matrix{Bool} since there it would apply to all of the entries). The rest of triage doesn't care.

Not sure I understand @oscardssmith: show for Matrix{Bool} already applies a transformation to its entries so that they're easier to read? or do you mean that it uses a different mechanism?

[jmichel7]

I don't know what means "too fancy", but people who look often at rational matrices, like mathematicians, do care.
See issue 4626

[StefanKarpinski]

It's also not clear that this is actually an improvement in looking at these matrices. For example, it messes up the alignment. It also doesn't really save space unless all the values happen to be integral (or the integral values are the only ones that are large, which seems unlikely). Is the subjective benefit to highlight integer-valued entries and make them easier to notice?

[jmichel7]

At least, it makes the position of the zero entries much easier to find. As for messing the alignment, it is a more general problem
(which I would claim is an independent issue) about printing some kinds of number arrays.

[simeonschaub]

It's also not clear that this is actually an improvement in looking at these matrices. For example, it messes up the alignment. It also doesn't really save space unless all the values happen to be integral (or the integral values are the only ones that are large, which seems unlikely). Is the subjective benefit to highlight integer-valued entries and make them easier to notice?

To me, the main advantage is that it reduces visual noise. Here is an example I just encountered:

julia> G⁻¹ = [j > i ? 0 : (-1)^(i+j) * binomial(i, j-1) for i in 1:10, j in 1:10];

julia> G = inv(Rational.(G⁻¹))
10×10 Matrix{Rational{Int64}}:
  1//1    0//1    0//1   0//1    0//1    0//1   0//1  0//1  0//1  0//1
  1//2    1//2    0//1   0//1    0//1    0//1   0//1  0//1  0//1  0//1
  1//6    1//2    1//3   0//1    0//1    0//1   0//1  0//1  0//1  0//1
  0//1    1//4    1//2   1//4    0//1    0//1   0//1  0//1  0//1  0//1
 -1//30   0//1    1//3   1//2    1//5    0//1   0//1  0//1  0//1  0//1
  0//1   -1//12   0//1   5//12   1//2    1//6   0//1  0//1  0//1  0//1
  1//42   0//1   -1//6   0//1    1//2    1//2   1//7  0//1  0//1  0//1
  0//1    1//12   0//1  -7//24   0//1    7//12  1//2  1//8  0//1  0//1
 -1//30   0//1    2//9   0//1   -7//15   0//1   2//3  1//2  1//9  0//1
  0//1   -3//20   0//1   1//2    0//1   -7//10  0//1  3//4  1//2  1//10

julia> map(x -> isinteger(x) ? Int(x) : x, G)
10×10 Matrix{Real}:
   1       0       0      0       0       0      0     0     0     0
  1//2    1//2     0      0       0       0      0     0     0     0
  1//6    1//2    1//3    0       0       0      0     0     0     0
   0      1//4    1//2   1//4     0       0      0     0     0     0
 -1//30    0      1//3   1//2    1//5     0      0     0     0     0
   0     -1//12    0     5//12   1//2    1//6    0     0     0     0
  1//42    0     -1//6    0      1//2    1//2   1//7   0     0     0
   0      1//12    0    -7//24    0      7//12  1//2  1//8   0     0
 -1//30    0      2//9    0     -7//15    0     2//3  1//2  1//9   0
   0     -3//20    0     1//2     0     -7//10   0    3//4  1//2  1//10

Can you say anything about the matrix structure of G by glancing at the first output?

From the second output OTOH you immediately recognize that the upper triangle is just all zeros and that there's some kind of band structure with occasional all-zero bands going on in the lower triangle. I really think this would be a big improvement in legibility and this seems very much in line with the decision made in #30575.

[jariji]

In @simeonschaub 's example, an alternative change would solve the issue of showing the matrix structure: show 0 instead of 0//d when the numerator is zero.

[jmichel7]

Some matrix structure can be seen easily only by showing 1 instead 1//1. There is really no reason to restrict this
simpler show only to some integers.

[LilithHafner]

From triage 👍

p.s. make sure to make it print correctly for matrices with a column of integers.

[kalmarek]

From triage +1

p.s. make sure to make it print correctly for matrices with a column of integers.

@LilithHafner I just built it again and get these results:

julia> using Random; Random.seed!(1)
TaskLocalRNG()

julia> x = rand(-3:3, 5,5).//rand(1:6, 5, 5)
5×5 Matrix{Rational{Int64}}:
 -1//2  -2//3    0     1//4    0
 -1//3   2//3    0    -2//3    1
  1//6   2//5  -1//3  -3//5  -1//3
  1//6   1//4   -3    -1//3    1
  3//5  -2//3    0     -1      0


julia> x[:, 1] .= 0;

julia> x
5×5 Matrix{Rational{Int64}}:
 0  -2//3    0     1//4    0
 0   2//3    0    -2//3    1
 0   2//5  -1//3  -3//5  -1//3
 0   1//4   -3    -1//3    1
 0  -2//3    0     -1      0

julia> x[:, 2] .= -1;

julia> x
5×5 Matrix{Rational{Int64}}:
 0  -1    0     1//4    0
 0  -1    0    -2//3    1
 0  -1  -1//3  -3//5  -1//3
 0  -1   -3    -1//3    1
 0  -1    0     -1      0

julia> x[:, end] = rand(-4:4, 5);

julia> x
5×5 Matrix{Rational{Int64}}:
 0  -1    0     1//4   2
 0  -1    0    -2//3  -4
 0  -1  -1//3  -3//5   0
 0  -1   -3    -1//3  -3
 0  -1    0     -1     0

To me it seems that everything runs as intended;
Should I add more tests? Did you have any particular test in mind?

[LilithHafner]

Spacing seems fine. I'd like a test for sprint(show, MIME"text/plain"(), Union{Int, Rational{Int}}[7 3//6; 6//3 2]), though.

[LilithHafner]

The new test is failing. Showing Union{Int, Rational{Int}}[7 3//6; 6//3 2]) currently does not skip showing the denominator, which I think is good. However the test tests that it does skip showing the denominator. I think the test's expected output should be changed to the current actual output.

[kalmarek]

I would have swore I did run the tests before committing ;)

[LilithHafner]

Aside from "needs news" (add this to https://github.com/JuliaLang/julia/blob/master/NEWS.md), this LGTM

[LilithHafner]

Internet-based tests failed across the board; merging master in to rerun tests.

[LilithHafner]

Thanks for your contribution and your persistence, @kalmarek! This is a nice improvement in readability in some cases.

[kalmarek]

@LilithHafner thanks for leading the final push ;)

I understand that since 1.9rc1 has already landed this will end up in 1.10?

[oscardssmith]

correct.

[kalmarek]

It's a pitty to miss the window by a few days, but better late than never ;)

[oscardssmith]

this was well past the window. We branched 1.9 in November.

[kalmarek]

good to know