Opened 9 years ago

Closed 9 years ago

# A patch adding .dimensions() to a matrix.

Reported by: Owned by: blundell tbd trivial sage-5.0 linear algebra Matrices sage-5.0.beta7 Benjamin Lundell William Stein, Daniel Krenn N/A

### 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).

### comment:1 follow-up: ↓ 2 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

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)
**********************************************************************
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: ↓ 8 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

"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.