Opened 8 years ago

Closed 4 years ago

#11968 closed defect (fixed)

bug in documentation of random_matrix

Reported by: AlexGhitza Owned by: jason, was
Priority: minor Milestone: sage-6.10
Component: linear algebra Keywords: random matrix documentation
Cc: kcrisman, wstein Merged in:
Authors: Rob Beezer Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: fad7862 (Commits) Commit: fad78622358f6fe9bc5cbe8efffe6b893e5e3969
Dependencies: Stopgaps:

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?

Change History (14)

comment:1 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:2 Changed 7 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 7 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 7 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:5 Changed 6 years ago by kcrisman

  • Cc kcrisman added

comment:6 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 Changed 5 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 4 years ago by rbeezer

  • Authors set to Rob Beezer
  • Branch set to u/rbeezer/random-matrix-zeros-doc
  • Cc wstein added
  • Commit set to fad78622358f6fe9bc5cbe8efffe6b893e5e3969
  • Keywords documentation added
  • 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:

fad7862Trac 11968: improve documentation of random matrix construction

comment:12 Changed 4 years ago by jmantysalo

  • Reviewers set to Jori Mäntysalo

I can check this.

comment:13 Changed 4 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 4 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.