Opened 3 years ago
Last modified 13 days 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.7 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
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 (9)
comment:1 Changed 2 years ago by
- Milestone changed from sage-9.0 to sage-9.1
comment:2 Changed 2 years ago by
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:
- Make changes in the
random_matrix()
function:
I could check whether kwds has the value
num_pivots
. If it doesn't, I will callrandom_rref_matrix
with a random number in the range [0, min(nrows, ncols)]. Will also have to check ifncols
is there or not.
I guess this is safe but will make the code look a little ugly.
- 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 ofnum_pivots
and assignnum_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 2 years ago by
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 2 years ago by
- 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 19 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:6 Changed 13 months ago by
- 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 9 months ago by
- Milestone changed from sage-9.4 to sage-9.5
comment:8 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:9 Changed 13 days ago by
- Milestone changed from sage-9.6 to sage-9.7
Ticket retargeted after milestone closed