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

Implement rank for sparse integer matrix using LinBox #25257

Closed
tscrim opened this issue Apr 29, 2018 · 18 comments
Closed

Implement rank for sparse integer matrix using LinBox #25257

tscrim opened this issue Apr 29, 2018 · 18 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Apr 29, 2018

Right now the only way to compute the rank of a sparse integer matrix is to either convert it to a dense matrix or a rational matrix (which simply does Gaussian elimination). Both of these options are horrible. Linbox provides a rank algorithm more for sparse matrices. The aim of this ticket is to expose this.

For example, I have a sparse matrix

matrix dims: 11550 x 5775
density: 1/1155
number nonzero positions: 57750

it takes <11s with linbox on my computer, and I gave up after well over minute doing it over Q.

This is a part of #13915.

CC: @sagetrac-Bouillaguet @ClementPernet @videlec

Component: linear algebra

Keywords: linbox, sparse matrix

Reviewer: Vincent Delecroix

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

@tscrim tscrim added this to the sage-8.2 milestone Apr 29, 2018
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 29, 2018

comment:1

Here is an initial branch that is horribly hacked together, but it was sufficient for my purposes and to demonstrate that we should do this.


New commits:

afc4263Initial hack of LinBox's rank for sparse matrices.

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 29, 2018

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 29, 2018

Commit: afc4263

@videlec
Copy link
Contributor

videlec commented Apr 29, 2018

comment:2

Nice.

Please add tests for all possible corner cases (0 x 1, 1 x 0 and 0 x 0). I had troubles with these with other linbox functions.

I think that it would be better to isolate the conversion dict -> sparse matrix as in #24544 (libs/linbox/conversion.pxd). What do you think? Moreover, the datastructure for sparse integer matrix is not a dict. Converting to a dict is a lot of waste. Though, this can be done in a second time.

@videlec
Copy link
Contributor

videlec commented Apr 29, 2018

comment:3

BTW, I already have rank/det/solve for sparse matrices on a branch. You might want to wait for next week (Sage days in Cernay).

@videlec
Copy link
Contributor

videlec commented Apr 29, 2018

comment:4

(my version has C++ conversions between the mpz_vector * of Sage and linbox custom datatype)

@videlec
Copy link
Contributor

videlec commented Apr 29, 2018

comment:5

But priority is #24544 which fails to compile :-(

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 29, 2018

comment:6

I completely agree with comment:2. I was just trying to get the rank of certain big matrices rather than do rank frequently, so converting from the internal format was not a big issue for me. Again "horribly hacked together" :) I also highly doubt what I've done is the correct, or even good, way to do it too.

I am happy to wait until next week. I will be visiting Sydney, so I probably won't have much time to devote to Sage development next week. However, I am happy to review tickets where I can.

@videlec
Copy link
Contributor

videlec commented May 1, 2018

comment:7

Could you try #23214?

@tscrim
Copy link
Collaborator Author

tscrim commented May 1, 2018

Changed author from Travis Scrimshaw to none

@tscrim
Copy link
Collaborator Author

tscrim commented May 1, 2018

Changed commit from afc4263 to none

@tscrim
Copy link
Collaborator Author

tscrim commented May 1, 2018

Changed branch from public/linear_algebra/linbox_rank_sparse_matrix-25257 to none

@tscrim
Copy link
Collaborator Author

tscrim commented May 1, 2018

comment:8

I would say #23214 superseeds this, which we can close as a duplicate.

@tscrim tscrim removed this from the sage-8.2 milestone May 1, 2018
@videlec
Copy link
Contributor

videlec commented May 1, 2018

comment:9

Thanks for creating this ticket: it motivated me to finish #23214.

@videlec
Copy link
Contributor

videlec commented May 1, 2018

Reviewer: Vincent Delecroix

@tscrim
Copy link
Collaborator Author

tscrim commented May 1, 2018

comment:10

Thank you for doing a much better job than I did on this ticket. I will try to get to #23214 as soon as I can. I think I have some time in the afternoon tomorrow to properly review it then (too tired tonight to review the #24544 parts).

@videlec
Copy link
Contributor

videlec commented May 18, 2018

comment:11

closing positively reviewed duplicates

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

2 participants