Opened 9 years ago

Closed 9 years ago

#12604 closed enhancement (fixed)

A patch adding .dimensions() to a matrix.

Reported by: blundell Owned by: tbd
Priority: trivial Milestone: sage-5.0
Component: linear algebra Keywords: Matrices
Cc: Merged in: sage-5.0.beta7
Authors: Benjamin Lundell Reviewers: William Stein, Daniel Krenn
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This is a simple patch which gives you the dimensions of a matrix. So if M has 3 rows and 5 columns, M.dimensions() returns the tuple (3,5).

Attachments (1)

trac_12604.patch (1.0 KB) - added by was 9 years ago.
apply only this patch

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: Changed 9 years ago by dsm

  • Status changed from new to needs_info

I'm not sure we can use ".dimensions()" for this, at least by itself. Our terminology is already a little weird.. in the parent MatrixSpace?, we have:

sage: m = Matrix(QQ, 3, 5)
sage: m
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]
sage: parent(m)
Full MatrixSpace of 3 by 5 dense matrices over Rational Field
sage: ms = parent(m)
sage: ms.dim 
ms.dimension  ms.dims       
sage: ms.dims()
(3, 5)
sage: ms.dimension()
15

Those aren't the names I'd have chosen, but given that they're there, we shouldn't have .dims() work for the MatrixSpace? and .dimensions() work for an element in the space.

.dimensions() is a better name, IMHO, so maybe we should make both .dimensions() and .dimension() work for the MatrixSpace? and for a Matrix, and let .dims() be an alias for .dimensions() for backward compatibility purposes. Above my pay grade, though.

comment:2 in reply to: ↑ 1 Changed 9 years ago by was

  • Status changed from needs_info to needs_review

Replying to dsm:

I'm not sure we can use ".dimensions()" for this, at least by itself.

I personally can not remember using or hearing the word "dimensions" to refer to the number of rows and columns of a matrix. However, I just did a google search, and this is *definitely* very standard terminology all over the place. So I think it is very sensible to add this.

Our terminology is already a little weird.. in the parent MatrixSpace?, we have:

sage: m = Matrix(QQ, 3, 5)
sage: m
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]
sage: parent(m)
Full MatrixSpace of 3 by 5 dense matrices over Rational Field
sage: ms = parent(m)
sage: ms.dim 
ms.dimension  ms.dims       
sage: ms.dims()
(3, 5)

It would be good to have dims = dimensions as an alias for consistency. Don't just change it since that will break code.

sage: ms.dimension() 15 }}}

ms.dimension() is literally the dimension of the matrix space itself. This has nothing to do with "dimensions()", except that it is the product of them.

Those aren't the names I'd have chosen, but given that they're there, we shouldn't have .dims() work for the MatrixSpace? and .dimensions() work for an element in the space.

.dimensions() is a better name, IMHO, so maybe we should make both .dimensions() and .dimension() work for the MatrixSpace? and for a Matrix, and let .dims() be an alias for .dimensions() for backward compatibility purposes. Above my pay grade, though.

I totally agree with your conclusion. But I'm not convinced that changes to MatrixSpace? should be part of this ticket. It could be in another ticket.

Changed 9 years ago by was

apply only this patch

comment:3 Changed 9 years ago by was

REFEREE REPORT:

  1. It fails all the doctests:
    wstein@sage:/scratch/wstein/sage-5.0.beta5-sage.math.washington.edu-x86_64-Linux/devel/sage/sage$ sage -t matrix/matrix0.pyx 
    sage -t  "devel/sage-main/sage/matrix/matrix0.pyx"          
    **********************************************************************
    File "/mnt/usb1/scratch/wstein/sage-5.0.beta5-sage.math.washington.edu-x86_64-Linux/devel/sage-main/sage/matrix/matrix0.pyx", line 2028:
        sage: M.dimensions()
    Expected:
        (2,3)
    Got:
        (2, 3)
    **********************************************************************
    File "/mnt/usb1/scratch/wstein/sage-5.0.beta5-sage.math.washington.edu-x86_64-Linux/devel/sage-main/sage/matrix/matrix0.pyx", line 2030:
        sage: N.dimensions()
    Expected:
        (3,2)
    Got:
        (3, 2)
    **********************************************************************
    1 items had failures:
       2 of   7 in __main__.example_35
    ***Test Failed*** 2 failures.
    For whitespace errors, see the file /scratch/wstein/sage//tmp/matrix0_29905.py
             [6.3 s]
    
  1. The description in the patch should probably be "Trac #12604: Added a method to a matrix which gives the dimensions."
  1. The author should be "Ben Lundell" instead of math480 student.

I've attached a patch fixing all these issues.

comment:4 Changed 9 years ago by was

  • Authors set to Benjamin Lundell
  • Keywords Matrices added; Matricies removed
  • Reviewers set to William Stein

Can somebody sign off on my review patch?

comment:5 follow-up: Changed 9 years ago by dsm

"ms.dimension() is literally the dimension of the matrix space itself. This has nothing to do with "dimensions()", except that it is the product of them."

This is a use of the word "nothing" with which I am not familiar. :-)

comment:6 Changed 9 years ago by dkrenn

  • Status changed from needs_review to positive_review

comment:7 Changed 9 years ago by dkrenn

  • Reviewers changed from William Stein to William Stein, Daniel Krenn

comment:8 in reply to: ↑ 5 Changed 9 years ago by was

Replying to dsm:

"ms.dimension() is literally the dimension of the matrix space itself. This has nothing to do with "dimensions()", except that it is the product of them."

This is a use of the word "nothing" with which I am not familiar. :-)

These are not the Droids you are looking for.

comment:9 Changed 9 years ago by jdemeyer

  • Component changed from PLEASE CHANGE to linear algebra

comment:10 Changed 9 years ago by jdemeyer

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