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:

Status badges

Description (last modified by jdemeyer)

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)

trac_10541_vectors_to_matrices.patch (5.4 KB) - added by rbeezer 11 years ago.
trac_10541_deprecate_vector_transpose.patch (3.5 KB) - added by rbeezer 11 years ago.
trac_10541-doctests-conics.patch (1.8 KB) - added by rbeezer 11 years ago.
trac_10541-doctest-empty-column.patch (855 bytes) - added by mstreng 11 years ago.
use all 4 patches in order
trac_10541-doctest-triangulation.patch (841 bytes) - added by rbeezer 11 years ago.
Apply all 5 patches, in order

Download all attachments as: .zip

Change History (28)

Changed 11 years ago by rbeezer

Changed 11 years ago by rbeezer

comment:1 Changed 11 years ago by rbeezer

  • Authors set to Rob Beezer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 11 years ago by rbeezer

  • Cc mhampton vbraun added

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 deprecate transpose for vectors (not matrices!), so it would need to become column and then once I figured out what was going on, it made sense to change the matrix constructor to row. 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

comment:3 Changed 11 years ago by vbraun

  • 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 rbeezer

Thanks, Volker! (That was fast!) It was Jason's suggestion, so he should take the credit.

comment:5 follow-up: Changed 11 years ago by jdemeyer

  • 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 rbeezer

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 rbeezer

comment:7 follow-up: Changed 11 years ago by rbeezer

  • 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: Changed 11 years ago by mstreng

  • 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 in con_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.

Changed 11 years ago by mstreng

use all 4 patches in order

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 11 years ago by mstreng

As the patchbot still uses 4.6.1, let's help it by mentioning that this ticket now requires #727.

Sorry patchbot, I meant "depends on #727".

All changes look good, but I'm not so sure about deprecating transpose for vectors. Did you ask on the mailing lists if people object?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 11 years ago by mstreng

  • 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: Changed 11 years ago by rbeezer

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: Changed 11 years ago by jdemeyer

  • Description modified (diff)

Replying to mstreng:

As the patchbot still uses 4.6.1, let's help it by mentioning that this ticket now requires #727.

Sorry patchbot, I meant "depends on #727".

PLEASE PLEASE PLEASE list this in the ticket description for HUMANS!!!

comment:13 in reply to: ↑ 12 ; follow-up: Changed 11 years ago by mstreng

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: Changed 11 years ago by mstreng

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 jdemeyer

Replying to mstreng:

Sorry Jeroen, I thought that wasn't neccessary as you've already merged #727.

Of course, in this particular case it's not needed, but it was more a general principle.

comment:16 in reply to: ↑ 14 ; follow-up: Changed 11 years ago by rbeezer

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: Changed 11 years ago by mstreng

  • 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 rbeezer

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 jdemeyer

  • 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)]
**********************************************************************

Changed 11 years ago by rbeezer

Apply all 5 patches, in order

comment:20 Changed 11 years ago by rbeezer

  • Status changed from needs_work to needs_review

Conflict comes from new code in #9918 as of 4.6.2.alpha3.

Latest patch converts an outer product to v.column()*v.row(), so tests now pass in the affected file.

Volker - do you want to check-off on this change to your code? You might also like the outer-product method waiting at #10545. ;-)

Rob

comment:21 follow-up: Changed 11 years ago by vbraun

  • Status changed from needs_review to positive_review

Thanks, looks good now.

comment:22 in reply to: ↑ 21 Changed 11 years ago by rbeezer

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 jdemeyer

  • Merged in set to sage-4.6.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.