Skip to content

Conversation

@rburing
Copy link
Contributor

@rburing rburing commented Aug 20, 2025

This uses less memory than the only alternative which is a dense option, so it can handle some bigger matrices.

The code is based on analogous existing/surrounding Sage code and LinBox's example nullspacebasis_rat.C.

I'm not yet fluent in Cythonese so comments are welcome.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@rburing

This comment was marked as resolved.

@rburing rburing force-pushed the sparse_matrix_QQ_right_kernel_LinBox branch 2 times, most recently from 99cc850 to f160365 Compare August 21, 2025 12:54
@github-actions
Copy link

github-actions bot commented Aug 21, 2025

Documentation preview for this PR (built with commit b67fb38; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@rburing rburing force-pushed the sparse_matrix_QQ_right_kernel_LinBox branch from f160365 to cd592ed Compare August 22, 2025 16:59
@user202729
Copy link
Contributor

otherwise looks good to me, but do add some doctest for the top-level method .right_kernel_matrix().

is the new algorithm asymptotically faster (for very sparse matrix)? If so it may be beneficial to add a test that can be handled quickly with the change, but cannot be handled in reasonable time without it.

@rburing rburing force-pushed the sparse_matrix_QQ_right_kernel_LinBox branch from cd592ed to 0a9d26a Compare August 25, 2025 14:30
@rburing rburing requested a review from user202729 August 25, 2025 14:41
@whoami730
Copy link
Contributor

@rburing please avoid rebases / force pushes if possible. That messes with review history and makes reviews more cumbersome.

@rburing
Copy link
Contributor Author

rburing commented Aug 25, 2025

please avoid rebases / force pushes if possible.

I'll keep it in mind for next time.

I didn't change the base commit, so you can just press the Compare link to see the changes since the previous push.

@user202729
Copy link
Contributor

user202729 commented Aug 25, 2025

bug:

matrix([
	[ZZ.random_element(-5, 5)/ZZ.random_element(1, 5) if random() < 1/20 else 0 for j in range(400)]
	for i in range(200)
	]).right_kernel_matrix(algorithm="linbox")

run it, then ctrl-C, you'll see it's spending time in flint or iml. When the original matrix is dense, the algorithm= parameter is silently thrown away.

even when it's sparse, sometimes it spend time in echelonize_multimodular.

alarm(0.3)
matrix([
	[ZZ.random_element(-5, 5)/ZZ.random_element(1, 5) if random() < 1/20 else 0 for j in range(200)]
	for i in range(100)
	], sparse=True).right_kernel_matrix(algorithm="linbox")
cancel_alarm()

I'm also struggling to find a combination of parameters that makes linbox faster than the other algorithms. (although if the goal is to provide an interface, this is not mandatory.)

@rburing
Copy link
Contributor Author

rburing commented Aug 26, 2025

I improved the error handling.

The reason you see it spending time on echelonization with other algorithms is because of the basis argument of right_kernel_matrix defaulting to 'echelon' in this case, which does echelonization after computing the basis with the specified algorithm. To avoid that, you can pass e.g. basis = 'computed'.

The example I used is a 38875 x 13867 matrix ec9.txt (in SMS format) that you can load:

def read_sms_matrix(filename):
    f = open(filename)
    firstline = f.readline()
    dimensions = firstline.split(" ")
    rows = int(dimensions[0])
    columns = int(dimensions[1])
    A = zero_matrix(QQ, rows, columns, sparse=True)
    for line in f:
        parts = [int(z) for z in line.split(" ")]
        row = parts[0] - 1
        column = parts[1] - 1
        entry = parts[2]
        if row == 0 and column == 0 and entry == 0:
            break
        A[row, column] = entry
    f.close()
    return A

Trying the right_kernel_matrix method on this matrix runs out of memory with the dense algorithm.

@user202729
Copy link
Contributor

looks hard to make this into a doctest (unless someone can figure out how to concisely construct a matrix with similar property), but otherwise should be fine.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 1, 2025
sagemathgh-40650: Add LinBox algorithm for right kernel of sparse matrix over the rationals
    
This uses less memory than the only alternative which is a dense option,
so it can handle some bigger matrices.

The code is based on analogous existing/surrounding Sage code and
LinBox's example [`nullspacebasis_rat.C`](https://github.com/linbox-team
/linbox/blob/d93e497b6233175addf34891ca8f7a37aa4b2f41/examples/nullspace
basis_rat.C).

I'm not yet fluent in Cythonese so comments are welcome.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40650
Reported by: Ricardo Buring
Reviewer(s): Ricardo Buring, Sahil Jain, user202729
@vbraun vbraun merged commit 65ec458 into sagemath:develop Sep 7, 2025
25 of 28 checks passed
@rburing rburing deleted the sparse_matrix_QQ_right_kernel_LinBox branch October 17, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants