Opened 11 years ago
Closed 11 years ago
#10541 closed enhancement (fixed)
Conversions of vectors to matrices
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.6.2 |
Component: | linear algebra | Keywords: | |
Cc: | jason, mhampton, vbraun, mstreng | Merged in: | sage-4.6.2.alpha4 |
Authors: | Rob Beezer | Reviewers: | Volker Braun, Marco Streng |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
A vector is naturally a single-row matrix or a single-column matrix. There is an implicit assumption in Sage that the former is more desirable. These patches aim to put the two options on even footing, while also making the conversion of the type perhaps more obvious.
See the discussion on sage-devel.
First patch: implements .row() and .column() for vectors, as suggested by Jason Grout. There is a nice symmetry with the commands .row(i) and .column(i) for matrices, see the doctests. Also allows sparseness to be preserved.
Second patch: deprecates .transpose() which presumes rows are preferable and has its functionality exactly duplicated by .column(). Called once in a number field doctest, apparently for purely cosmetic reasons. Called once in polyhedra code, presumably in an outer product (which should also be a new method for vectors). Details on this in a comment.
Dependency: #727
Attachments (5)
Change History (28)
Changed 11 years ago by
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Cc mhampton vbraun added
comment:3 Changed 11 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
I like the syntax! It definitely makes the code easier to read. Positive review.
comment:4 Changed 11 years ago by
Thanks, Volker! (That was fast!) It was Jason's suggestion, so he should take the credit.
comment:5 follow-up: ↓ 6 Changed 11 years ago by
- Status changed from positive_review to needs_work
There are various doctest failures (not surprisingly, since you deprecated an existing and used function).
comment:6 in reply to: ↑ 5 Changed 11 years ago by
Replying to jdemeyer:
There are various doctest failures (not surprisingly, since you deprecated an existing and used function).
Thanks, Jeroen.
I know I tested this against the whole Sage library when I built it, and I just tested it against 4.6.1.alpha3 on the Sage library code with no failures. So maybe it is premature bitrot?
I need to get my dev tree caught up to the latest alpha, and then I will test this again against *everything* and track down the problem.
Sorry for the trouble.
Rob
Changed 11 years ago by
comment:7 follow-up: ↓ 8 Changed 11 years ago by
- Cc mstreng added
- Status changed from needs_work to needs_review
Failures are due to some new "transposes" of vectors that slipped into 4.6.2.alpha2 as part of #727. Latest patch changes those to columns, so doctests now pass in all of sage/schemes
.
Marco - do you want to check-off on just the changes in the latest patch? Localized to just the diagonal_matrix()
method in con_field.py
. Thanks.
Needs all three patches for full testing.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 11 years ago by
- Description modified (diff)
Replying to rbeezer:
Marco - do you want to check-off on just the changes in the latest patch? Localized to just the
diagonal_matrix()
method incon_field.py
. Thanks.
Those changes are fine.
By the way, on line 767 of the first page, you write sage: w = v.row()
as an example of column, so row
should be column
. I suppose 2 lines later it would then be a 0 by 1
matrices instead of 1 by 0
.
As the patchbot still uses 4.6.1, let's help it by mentioning that this ticket now requires #727.
comment:9 in reply to: ↑ 8 ; follow-ups: ↓ 10 ↓ 12 Changed 11 years ago by
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 11 years ago by
- Reviewers changed from Volker Braun to Volker Braun, Marco Streng
All changes look good,
and all tests pass, so I would give it a positive review, except that I just uploaded a patch here myself and
I'm not so sure about deprecating
transpose
for vectors. Did you ask on the mailing lists if people object?
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 14 Changed 11 years ago by
Replying to mstreng:
All changes look good,
and all tests pass, so I would give it a positive review, except that I just uploaded a patch here myself and
I'm not so sure about deprecating
transpose
for vectors. Did you ask on the mailing lists if people object?
Hi Marco,
Thanks for attention to this - I was going to get back to it later tonight.
In Sage, a vector does not have an orientation, just an implicit assumption that it should be treated like a row of a matrix if you have to make a decision. I am trying to make the linear algebra code much easier for beginners to use, and then integrating all that into my open-source introductory textbook. You can see progress so far at: http://wiki.sagemath.org/devel/LatexToWorksheet
So I feel strongly that just having a "vector transpose" is a source of confusion, and instead having row() and a column() methods will make it much more clear what is happening - and column() is a drop-in replacement. This came up at: http://groups.google.com/group/sage-devel/browse_thread/thread/623226316555c28e/
You will see from the deprecation patch that .transpose()
was hardly ever used anywhere.
I do not think the doctest in question is incorrect. It wasn't failing was it? Patch implements .row()
which converts a vector to a 1 x n matrix. So an empty vector should become a 1 x 0 matrix as a row. Maybe you would like a similar test for .column()
? And maybe that would be better than the one that is there. Maybe having both would be the best solution.
Rob
comment:12 in reply to: ↑ 9 ; follow-up: ↓ 13 Changed 11 years ago by
- Description modified (diff)
comment:13 in reply to: ↑ 12 ; follow-up: ↓ 15 Changed 11 years ago by
Replying to jdemeyer:
PLEASE PLEASE PLEASE list this in the ticket description for HUMANS!!!
Sorry Jeroen, I thought that wasn't neccessary as you've already merged #727. I'll put stuff like that in the description from now on
Anyway, I think your request should be part of the documentation for trac (see #10702), instead of just in tickets! Or do capitals make you heard outside this ticket as well?
comment:14 in reply to: ↑ 11 ; follow-up: ↓ 16 Changed 11 years ago by
Replying to rbeezer:
So I feel strongly that just having a "vector transpose" is a source of confusion,
Hi Rob,
Thanks for the explanation. I know most of that already, and personally do agree with it.
On the other hand, some people may have used vector transpose
a lot in their own code, or be strongly opposed to this change for other reasons. You did ask if there were any objections, but that was in the middle of a long thread with title 'Adjoint of a matrix'. I'm not sure if people got enough chance to object.
I do not think the doctest in question is incorrect. It wasn't failing was it?
It wasn't incorrect in that sense, it just occurred twice, and tested the wrong function in the second occurrence. Both .row()
and .column()
have the same doctest
sage: v = vector(ZZ, []) sage: w = v.row() sage: w.parent() Full MatrixSpace of 1 by 0 dense matrices over Integer Ring
It would make more sense to replace v.row()
by v.column()
in the doctest of column
instead of repeating the doctest of row
. I assumed that is what you meant to do in your first patch, so it is what I did in mine.
Marco
comment:15 in reply to: ↑ 13 Changed 11 years ago by
comment:16 in reply to: ↑ 14 ; follow-up: ↓ 17 Changed 11 years ago by
Replying to mstreng:
On the other hand, some people may have used vector
transpose
a lot in their own code, or be strongly opposed to this change for other reasons. You did ask if there were any objections, but that was in the middle of a long thread with title 'Adjoint of a matrix'. I'm not sure if people got enough chance to object.
Well, I knew that adjoint/adjugate stuff would provoke discussion. So I did the right thing and worked hard on that long thread. I thought I had a consensus and did all the work on a patch. Only when I consulted authors of affected code did I get an objection. The patch got switched to "needs_work" - but with no suggestions and apparently just a suggestion. I don't see this change as so controversial. But once burned, twice shy.
If you wanted to take it to the list, I would not object. Of course, it is not your resposnsibility. I just am not too enthusiastic about going down that road again right now. I'm still unsure about what to do with the adjoint mess.
It wasn't incorrect in that sense, it just occurred twice, and tested the wrong function in the second occurrence.
OK, my fault. Sorry. I just read your change and did not read it in context. So I understand now, and appreciate the catch and your contribution.
Rob
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 11 years ago by
- Status changed from needs_review to positive_review
Replying to rbeezer:
I just am not too enthusiastic about going down that road again right now.
I can understand that. I guess people who missed out on that particular discussion can object during the deprecation period. I was already satisfied with your 3 patches, and I will interpret
appreciate the catch and your contribution.
as a positive review for my own (trivial) patch.
comment:18 in reply to: ↑ 17 Changed 11 years ago by
Replying to mstreng:
Hi Marco,
Thanks again for the catch on the doctest and understanding the dangerous waters of deprecating *anything*. ;-) It will, of course, be easy to rollback this deprecation and downgrade it to some sort of advisory in the documentation. I'll be at this project for some time, so I'll keep an eye on it.
Rob
comment:19 Changed 11 years ago by
- Status changed from positive_review to needs_work
Doctest failure:
sage -t -force_lib devel/sage/sage/geometry/triangulation/point_configuration.py ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.6.2.alpha4/devel/sage-main/sage/geometry/triangulation/point_configuration.py", line 1510: sage: pyramid.restricted_automorphism_group() Expected: Permutation Group with generators [(3,5), (2,3)(4,5), (2,4)] Got: doctest:1529: DeprecationWarning: (Since Sage Version 4.6.2) The transpose() method for vectors has been deprecated, use column() instead (or check to see if you have a vector when you really want a matrix) Permutation Group with generators [(3,5), (2,3)(4,5), (2,4)] **********************************************************************
comment:20 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:21 follow-up: ↓ 22 Changed 11 years ago by
- Status changed from needs_review to positive_review
Thanks, looks good now.
comment:22 in reply to: ↑ 21 Changed 11 years ago by
Replying to vbraun:
Thanks, looks good now.
Thanks for the quick look. Maybe (just maybe) this will get wrapped up on the current release!
comment:23 Changed 11 years ago by
- Merged in set to sage-4.6.2.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
Marshall, Volker: The deprecation patch touches
sage/geometry/polyhedra.py
at about line 4859. I changed (what appears) to be an outer product, because I would like to deprecatetranspose
for vectors (not matrices!), so it would need to becomecolumn
and then once I figured out what was going on, it made sense to change the matrix constructor torow
. 11 doctests failed prior to the change and all pass now. Would one of you like to double-check my changes?Both patches are necessary for the documentation to be right, one fixup ended up in the wrong patch. Otherwise they are self-contained, but need to be applied in order.
Patchbot:
Apply trac_10541_vectors_to_matrices.patch, trac_10541_deprecate_vector_transpose.patch