random_matrix shouldn't error out on missing num_pivots with algorithm='echelon_form'

[6bfcfeed-779b-4b16-82e6-63808dde0af0]

The following innocent code gives a TypeError:

e=random_matrix(ZZ,4,5,algorithm='echelon_form')

while I should have given:

e=random_matrix(ZZ,4,5,algorithm='echelon_form', num_pivots=3)

The code should just use a random value for the number of pivots when the argument isn't given.

Component: linear algebra

Issue created by migration from https://trac.sagemath.org/ticket/28520

[embray]

comment:1

Ticket retargeted after milestone closed

[7b2535dd-ca5f-44fd-abc7-513e35b94168]

comment:2

I would like to work on this issue. So, when either random_matrix() or matrix.random() is called with the keyword argument algorithm=echelon_form, the following piece of code gets run:

   def random_matrix(...):
      ...
      elif algorithm == 'echelon_form':
          return random_rref_matrix(parent, *args, **kwds)

This calls the following signature:

def random_rref_matrix(parent, num_pivots)

Both of these pieces of code are in the file: sage/local/lib/python3.7/site-packages/sage/matrix/special.py

Now I guess I have two options to fix it:

1. Make changes in the random_matrix() function: I could check whether kwds has the value num_pivots. If it doesn't, I will call random_rref_matrix with a random number in the range [0, min(nrows, ncols)]. Will also have to check if ncols is there or not.

I guess this is safe but will make the code look a little ugly.

2. Make changes to the random_rref_matrix() function: I will change the signature to include a *args and a **kwds. Check within the function for the existence of num_pivots and assign num_pivots appropriately if it is not there.

Will have to ensure the error messages would be the same as before? Seems a bit unsafe as a lot of other functions might call this function, but should be cleaner.

Which way should I proceed? Is there anything I'm missing?

[7b2535dd-ca5f-44fd-abc7-513e35b94168]

comment:3

Link to the Google groups discussion pertaining to this ticket: https://groups.google.com/forum/#!topic/sage-devel/Hvk1CeF8dw0

Essentially, the proposal in the previous comment I made would not work as it would not maintain the random distribution from which matrices are sampled (as matrices are not equally distributed for each 'number of pivots'). The solution was to either explicitly decide a distribution and write code to generate matrices from it (see Google groups discussion) or to default at num_pivots being equal to the maximum number of pivots.

I personally feel the former approach is better as it provides greater functionality - anyone who wants maximum number of pivots could just specify it, however, there currently exists no functionality to generate a completely random echelon matrix.

Hopefully this helps whoever picks up this ticket.

[mkoeppe]

comment:4

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[mkoeppe]

comment:6

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.