Opened 10 years ago

Last modified 8 years ago

#11715 closed enhancement

Upgrade matrix set_row() and set_column() — at Version 10

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

Status badges

Description (last modified by kcrisman)

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.

Change History (11)

comment:1 Changed 10 years ago by rbeezer

  • Authors set to Rob Beezer
  • Keywords beginner added
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by rbeezer

  • Keywords sd32 added

comment:3 Changed 10 years ago by jason

  • Cc jason added

comment:4 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to 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 8 years ago by hds

  • Cc hds added
  • Status changed from needs_work to needs_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 8 years ago by kcrisman

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 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman, David Loeffler
  • Status changed from needs_review to needs_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 8 years ago by hds

  • Reviewers changed from Karl-Dieter Crisman, David Loeffler to Karl-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 8 years ago by hds

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

comment:9 Changed 8 years ago by hds

  • Status changed from needs_work to needs_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 8 years ago by kcrisman

  • Description modified (diff)

Patchbot, apply trac_11715_matrix_set_row_column_docs_fixed.patch

Note: See TracTickets for help on using tickets.