Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add zero_matrix constructor. #2299

Closed
ncalexan mannequin opened this issue Feb 25, 2008 · 11 comments
Closed

add zero_matrix constructor. #2299

ncalexan mannequin opened this issue Feb 25, 2008 · 11 comments

Comments

@ncalexan
Copy link
Mannequin

ncalexan mannequin commented Feb 25, 2008

This adds the zero_matrix convenience constructor, just like identity_matrix.

CC: @ncalexan

Component: linear algebra

Keywords: zero matrix identity one

Issue created by migration from https://trac.sagemath.org/ticket/2299

@ncalexan ncalexan mannequin added this to the sage-2.10.3 milestone Feb 25, 2008
@ncalexan ncalexan mannequin assigned malb Feb 25, 2008
@ncalexan ncalexan mannequin added the s: needs review label Feb 25, 2008
@malb
Copy link
Member

malb commented Feb 25, 2008

comment:1

Attachment: 2299-ncalexan-zero-matrix-1.patch.gz

You replaced "n x n" by "$n \times n$" which reads nicer if typesetted. However, it is harder to comprehend from the command line (foo?).

I am not sure that we need identity_matrix and zero_matrix (i.e. the functional versions of MS(0) and MS(1)) but that is a matter of taste.

@ncalexan
Copy link
Mannequin Author

ncalexan mannequin commented Feb 25, 2008

comment:2

I really want identity_matrix and zero_matrix because I often don't want to name the matrix space in advance. MS(0) requires me to create said space first.

As for latex in docstrings... I don't care either way. I think it should be latex but I parse simple latex from tex without trouble.

@williamstein
Copy link
Contributor

comment:3

Just a quick remark. You can make a zero matrix using the matrix function:

sage: matrix(ZZ, 2,3)
[0 0 0]
[0 0 0]

I don't think that's a good reason not to add zero_matrix and identity_matrix
functions though, both of which I would like to have too.

@ncalexan
Copy link
Mannequin Author

ncalexan mannequin commented Feb 25, 2008

comment:4

I think zero_ and identity_ declare programmer intent nicely. Will someone say positive or negative and this either goes to the bit-bucket or gets applied?

@williamstein
Copy link
Contributor

comment:5

REFEREE REPORT:

I definitely think these functions should go in, but with TWO caveats.

  1. I think the zero_matrix should have an option to be nonsquare though. Please post another patch that adds that, so, e.g.,
sage: zero_matrix(ZZ, 2,3)
[0 0 0]
[0 0 0]

works.

  1. There should be a sparse option for both zero_matrix and
    identity_matrix.
sage: zero_matrix(ZZ, 2,3, sparse=True).parent()
Full MatrixSpace of 2 by 3 sparse matrices over Integer Ring

-- William

@williamstein williamstein changed the title add zero_matrix constructor. [needs update] add zero_matrix constructor. Feb 25, 2008
@williamstein williamstein changed the title [needs update] add zero_matrix constructor. [positive review pending updates] add zero_matrix constructor. Feb 25, 2008
@ncalexan
Copy link
Mannequin Author

ncalexan mannequin commented Feb 25, 2008

Attachment: 2299-ncalexan-zero-matrix-2.patch.gz

@ncalexan
Copy link
Mannequin Author

ncalexan mannequin commented Feb 25, 2008

comment:7

2299-ncalexan-zero-matrix-2.patch should apply without the previous patch, and should address the referee's comments.

I generated this patch using hg diff between two revisions, not hg export. Let me know if it's not okay.

@mwhansen
Copy link
Contributor

comment:8

Works for me in 2.10.3.alpha0. Note that only the last patch should be apply and that it is a "raw" patch.

@mwhansen mwhansen changed the title [positive review pending updates] add zero_matrix constructor. add zero_matrix constructor. Feb 27, 2008
@mwhansen
Copy link
Contributor

Attachment: 2299.patch.gz

@mwhansen
Copy link
Contributor

comment:9

I just realized this conflicts with #874. #874 should be applied first, and then 2299.patch should be applied.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Feb 28, 2008

comment:10

Merged 2299.patch in Sage 2.10.3.rc0

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Feb 28, 2008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants