Opened 10 years ago
Closed 9 years ago
#11715 closed enhancement (fixed)
Upgrade matrix set_row() and set_column()
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.9 |
Component: | linear algebra | Keywords: | beginner, sd32 |
Cc: | jason, hds | 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: |
Description (last modified by )
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)
Change History (16)
comment:1 Changed 10 years ago by
- Keywords beginner added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Keywords sd32 added
comment:3 Changed 10 years ago by
- Cc jason added
comment:4 Changed 10 years ago by
- Status changed from needs_review to needs_work
- Work issues set to doc formatting
comment:5 Changed 9 years ago by
- 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 9 years ago by
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 9 years ago by
- 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 9 years ago by
- 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.
comment:9 Changed 9 years ago by
- 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 9 years ago by
- Description modified (diff)
Patchbot, apply trac_11715_matrix_set_row_column_docs_fixed.patch
comment:11 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
- Status changed from needs_review to positive_review
Patchbot never did anything, but I checked on 5.8.rc0 and it's fine.
comment:14 Changed 9 years ago by
- Work issues doc formatting deleted
comment:15 Changed 9 years ago by
- Merged in set to sage-5.9.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
The following extract from the patch isn't valid Sphinx formatting (the first :: should be a single :)
There are a couple of these in the patch, see the output of "plugins.docbuild" in the patchbot logs.