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

elementary_divisors for integer matrices: fix doc string #5009

Closed
jhpalmieri opened this issue Jan 18, 2009 · 3 comments
Closed

elementary_divisors for integer matrices: fix doc string #5009

jhpalmieri opened this issue Jan 18, 2009 · 3 comments

Comments

@jhpalmieri
Copy link
Member

The doc string for the elementary_divisors method in matrix_integer_dense.pyx says

The elementary divisors are the invariants of the finite
abelian group that is the cokernel of this matrix. 

The word "cokernel" needs to be expanded upon. I think, from trial and error, that this is computing the cokernel of left multiplication by the matrix, and this needs to be clearly stated, especially given other left/right issues with matrices in Sage. (See #1587, for example.)

Furthermore, give at least one example where the matrix isn't square so we can see a bit more clearly on which side the matrix is acting, say a simple matrix like [[3, 0, 0], [0, 0, 0]]. Maybe even include both this and its transpose.

(As an editorial comment, I find it really annoying that methods like this are for left multiplication, while methods like restrict_codomain are for right multiplication, so if I want to use them both, I have to take transposes way too many times.)

Component: linear algebra

Keywords: elementary divisor

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

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 23, 2009

comment:1

Attachment: trac-5009.patch.gz

@rlmill rlmill mannequin added the s: needs review label Jan 23, 2009
@mwhansen
Copy link
Contributor

comment:2

Looks good to me.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Jan 28, 2009

comment:3

Merged in Sage 3.3.alpha3.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Jan 28, 2009
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