Opened 7 years ago
Closed 3 years ago
#11968 closed defect (fixed)
bug in documentation of random_matrix
Reported by:  AlexGhitza  Owned by:  jason, was 

Priority:  minor  Milestone:  sage6.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 7 years ago by
 Milestone sage4.7.3 deleted
comment:2 Changed 7 years ago by
 Milestone set to sage5.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
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
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
 Cc kcrisman added
comment:6 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:7 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:8 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:9 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 4 years ago by
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
 Branch set to u/rbeezer/randommatrixzerosdoc
 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:
fad7862  Trac 11968: improve documentation of random matrix construction

comment:13 Changed 3 years ago by
 Milestone changed from sage6.4 to sage6.10
 Status changed from needs_review to positive_review
LGTM.
comment:14 Changed 3 years ago by
 Branch changed from u/rbeezer/randommatrixzerosdoc to fad78622358f6fe9bc5cbe8efffe6b893e5e3969
 Resolution set to fixed
 Status changed from positive_review to closed
Milestone sage4.7.3 deleted