Opened 11 years ago

Closed 7 years ago

# bug in documentation of random_matrix

Reported by: Owned by: AlexGhitza jason, was minor sage-6.10 linear algebra random matrix documentation kcrisman, wstein Rob Beezer Jori Mäntysalo N/A fad7862 fad78622358f6fe9bc5cbe8efffe6b893e5e3969

### Description

The very first example in the docstring for `random_matrix` says

```Random integer matrices.  With no arguments, the majority of the
entries are -1 and 1, never zero, and rarely "large."

sage: random_matrix(ZZ, 5, 5)
[ -8   2   0   0   1]
[ -1   2   1 -95  -1]
[ -2 -12   0   0   1]
[ -1   1  -1  -2  -1]
[  4  -4  -6   5   0]
```

Hang on! What are those five zeroes doing in there?

### comment:1 Changed 11 years ago by jdemeyer

• Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

### comment:2 Changed 10 years ago by khalasz

• Milestone set to sage-5.1

The problem seems not to be that the given matrix contains 0s, but rather that the text above says it shouldn't contain 0s, as this command will many times give a matrix that indeed contains 0 entries. So, which should be fixed? The actual code or the wrong documentation? Is it that big of a deal to have a random matrix with 0 entries?

### comment:3 Changed 10 years ago by JoalHeagney

I think this is due to the default value of the density= keyword being changed from 1.0.

```A = random_matrix(ZZ,5); A

```
```[  2  -1   1  11   1]
[  0   1   1   1   1]
[  0   0   3   4   1]
[ 25   1   1   0 -27]
[ -2   1  -1   4  -2]

```
```A = random_matrix(ZZ,5,density=1.0); A

```
```[  2 -15 -23  -1  -4]
[  1  -3  -1   6  -1]
[  5   2  -2  -1  -1]
[ -1   2  -1   2   1]
[  1  -1  -2  -2   1]
```

If we're going to have a density keyword, then the documentation should at least state that you can create a matrix with no zero entries by setting density=1.0.

I've also created a ticket #13555 arguing for more doctesting of random functions.

### comment:4 Changed 10 years ago by JoalHeagney

Oh god, the documentation/behaviour isn't even consistent with x= and y= keywords.

```A = random_matrix(ZZ,5,x=4,y=10); A

[7 5 7 6 4]
[8 8 7 5 4]
[8 4 7 9 9]
[4 6 7 4 6]
[9 6 9 6 6]

```

So this gives us behaviour that agrees with the documentation, but seems to use density=1.0.

### comment:6 Changed 9 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:7 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:8 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:9 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:10 Changed 8 years ago by kcrisman

Just ran across this again. So clearly at odds with what is stated - yet another example:

```Also, the default is to not create zero entries,
unless the ``density`` keyword is set to something strictly less
than one.
```

What is going on?

```    if algorithm == 'randomize':
density = kwds.pop('density', None)
# zero matrix is immutable, copy is mutable
A = copy(parent.zero_matrix())
if density is None:
A.randomize(density=float(1), nonzero=False, *args, **kwds)
else:
A.randomize(density=density, nonzero=True, *args, **kwds)
return A
```

Okay, so this is the default. But oops - if `density is None` (default) then `nonzero=False`!!! So putting density in the right place is then frustrated by using the wrong nonzero value. And why is the other one `True`?

I would argue this is a bug, not bad documentation.

### comment:11 Changed 7 years ago by rbeezer

• Authors set to Rob Beezer
• Branch set to u/rbeezer/random-matrix-zeros-doc
• Status changed from new to needs_review

This contains only changes to the documentation, so that it matches the code. I've tried to clarify the documentation of the matrix `.randomize()` method so the action of the `density` and `nonzero` keywords are clearer. Hopefully their use in the `random_matrix()` constructor will then be clearer as well.

New commits:

 ​fad7862 `Trac 11968: improve documentation of random matrix construction`

### comment:12 Changed 7 years ago by jmantysalo

• Reviewers set to Jori Mäntysalo

I can check this.

### comment:13 Changed 7 years ago by jmantysalo

• Milestone changed from sage-6.4 to sage-6.10
• Status changed from needs_review to positive_review

LGTM.

### comment:14 Changed 7 years ago by vbraun

• Branch changed from u/rbeezer/random-matrix-zeros-doc to fad78622358f6fe9bc5cbe8efffe6b893e5e3969
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.