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: |
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)
Change History (11)
comment:1 follow-up: ↓ 2 Changed 9 years ago by
- Status changed from new to needs_info
comment:2 in reply to: ↑ 1 Changed 9 years ago by
- 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.
comment:3 Changed 9 years ago by
REFEREE REPORT:
- 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]
- The description in the patch should probably be "Trac #12604: Added a method to a matrix which gives the dimensions."
- 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
- 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
"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
- Status changed from needs_review to positive_review
comment:7 Changed 9 years ago by
- Reviewers changed from William Stein to William Stein, Daniel Krenn
comment:8 in reply to: ↑ 5 Changed 9 years ago by
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
- Component changed from PLEASE CHANGE to linear algebra
comment:10 Changed 9 years ago by
- Merged in set to sage-5.0.beta7
- Resolution set to fixed
- Status changed from positive_review to closed
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:
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.