Opened 11 years ago

Closed 10 years ago

#11715 closed enhancement (fixed)

Upgrade matrix set_row() and set_column()

Reported by: Rob Beezer Owned by: jason, was
Priority: minor Milestone: sage-5.9
Component: linear algebra Keywords: beginner, sd32
Cc: Jason Grout, Hayden Stainsby Merged in: sage-5.9.beta1
Authors: Rob Beezer, Hayden Stainsby Reviewers: Karl-Dieter Crisman, David Loeffler, Hayden Stainsby
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Karl-Dieter Crisman)

I thought the previous error message for set_row() about lists of entries being the wrong length was a bit vague, since it just referenced "v", which does not even need to be a vector.

So while changing that I upgaded the documentation and error-checking for both set_row() and set_column(). Attempting to use the set_unsafe() method resulted in a boatload of errors, so I backed off from that.


Apply only trac_11715_matrix_set_row_column_docs_fixed.patch.

Attachments (1)

trac_11715_matrix_set_row_column_docs_fixed.patch (6.4 KB) - added by Hayden Stainsby 10 years ago.
Updated set_row() and set_column for better error handling / reporting

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by Rob Beezer

Authors: Rob Beezer
Keywords: beginner added
Status: newneeds_review

comment:2 Changed 11 years ago by Rob Beezer

Keywords: sd32 added

comment:3 Changed 11 years ago by Jason Grout

Cc: Jason Grout added

comment:4 Changed 11 years ago by David Loeffler

Status: needs_reviewneeds_work
Work issues: doc formatting

The following extract from the patch isn't valid Sphinx formatting (the first :: should be a single :)

1617	1626	        EXAMPLES:: 
 	1627	         
 	1628	        New entries may be contained in a vector.  :: 

There are a couple of these in the patch, see the output of "plugins.docbuild" in the patchbot logs.

comment:5 Changed 10 years ago by Hayden Stainsby

Cc: Hayden Stainsby added
Status: needs_workneeds_review

I've updated Rob's patch file to fix the doc errors (I'm pretty sure there were just two). You only need to apply the new patch.

comment:6 Changed 10 years ago by Karl-Dieter Crisman

Well, here is the punishment for these good deeds.

Namely, if we're going to upgrade this documentation, we should probably also upgrade it to have tests for other things. Before there was no excuse, but now we should just do it all.

sage: A = matrix([[1, 2], [3, 4]]); A
[1 2]
[3 4]
sage: A.set_row(1,[.1,3])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<snip>
TypeError: Attempt to coerce non-integral RealNumber to Integer

Etc.

We should catch such errors and return a slightly more informative error message, just as we do with the wrong length list or wrong index, e.g. TypeError: new row must have all elements in original ring. Unless we want to allow this and do an implicit change_ring? Probably we should have an error message like in the doc for A.set_col_to_multiple_of_col?.


Minor comment - TESTS: needs two colons.

comment:7 Changed 10 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman, David Loeffler
Status: needs_reviewneeds_work

hds - can you put your real name under reviewers? You don't show up on the list at the main Trac page.

comment:8 Changed 10 years ago by Hayden Stainsby

Reviewers: Karl-Dieter Crisman, David LoefflerKarl-Dieter Crisman, David Loeffler, Hayden Stainsby

I added my name under reviewers, and on the main Trac page (hadn't done that yet as I only got the trac account today). Meanwhile, I'll look at some more informative errors for these coercion cases and such.

Changed 10 years ago by Hayden Stainsby

Updated set_row() and set_column for better error handling / reporting

comment:9 Changed 10 years ago by Hayden Stainsby

Status: needs_workneeds_review

I've added error handling for TypeErrors? that gives a more useful exception, suggesting that change_ring be used on the matrix first if the user wishes to set a row or column with elements from another ring.

sage: A = matrix(2, [1, 2, 3, 4])
sage: A.set_row(0, [1/3, 1]); A
Traceback (most recent call last):
...
TypeError: Cannot set row with Rational Field elements over Integer Ring, use change_ring first.

comment:10 Changed 10 years ago by Karl-Dieter Crisman

Description: modified (diff)

Patchbot, apply trac_11715_matrix_set_row_column_docs_fixed.patch

comment:11 Changed 10 years ago by Karl-Dieter Crisman

Authors: Rob BeezerRob Beezer, Hayden Stainsby

This looks pretty good and good work using Python 3 formatting :-)

Just waiting on the patchbot to make sure we're okay with respect to 5.8.beta4 or whatever the latest one is.

comment:12 Changed 10 years ago by Karl-Dieter Crisman

I can't figure out how to reset patchbot and get it to test...

Patchbot, apply only trac_11715_matrix_set_row_column_docs_fixed.patch

comment:13 Changed 10 years ago by Karl-Dieter Crisman

Status: needs_reviewpositive_review

Patchbot never did anything, but I checked on 5.8.rc0 and it's fine.

comment:14 Changed 10 years ago by Jeroen Demeyer

Work issues: doc formatting

comment:15 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.9.beta1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.