Opened 2 years ago

Last modified 4 weeks ago

#28520 new defect

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

Reported by: Snark Owned by:
Priority: major Milestone: sage-9.5
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (7)

comment:1 Changed 21 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:2 Changed 20 months ago by gh-Tinkidinki

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.

  1. 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?

comment:3 Changed 20 months ago by gh-Tinkidinki

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.

comment:4 Changed 17 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

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

comment:5 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:6 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:7 Changed 4 weeks ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5
Note: See TracTickets for help on using tickets.