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

Improve Matrix_mod2_dense solve_right performance #38843

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/sage/libs/m4ri.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ cdef extern from "m4ri/m4ri.h":
# reduced row echelon form using PLUQ factorization
cdef mzd_t *mzd_kernel_left_pluq(mzd_t *A, int cutoff)

# system solving
cdef int mzd_solve_left(mzd_t *A, mzd_t *B, int cutoff, int inconsistency_check)

########################
# Bit operations
########################
Expand Down
68 changes: 68 additions & 0 deletions src/sage/matrix/matrix_mod2_dense.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,74 @@ cdef class Matrix_mod2_dense(matrix_dense.Matrix_dense): # dense or sparse
self.cache('rank', r)
return r

def _solve_right_general(self, B, check=True):
"""
Solve the matrix equation AX = B for X using the M4RI library.

INPUT:

- ``B`` -- a matrix
- ``check`` -- boolean (default: ``True``); whether to check if the
matrix equation has a solution

EXAMPLES::

sage: A = matrix(GF(2), [[1, 0], [0, 1], [1, 1]])
sage: A.solve_right(vector([1, 1, 0]))
(1, 1)
sage: A.solve_right(vector([1, 1, 1]))
Traceback (most recent call last):
...
ValueError: matrix equation has no solutions

TESTS::

sage: n = 128
sage: m = 128
sage: A = random_matrix(GF(2), n, m)
sage: B = A * random_vector(GF(2), m)
sage: A * A.solve_right(B) == B
True
sage: m = 64
sage: A = random_matrix(GF(2), n, m)
sage: B = A * random_vector(GF(2), m)
sage: A * A.solve_right(B) == B
True
sage: m = 256
sage: A = random_matrix(GF(2), n, m)
sage: B = A * random_vector(GF(2), m)
sage: A * A.solve_right(B) == B
True
"""
cdef Matrix_mod2_dense X # the solution

cdef mzd_t *B_entries = (<Matrix_mod2_dense>B)._entries
cdef rci_t rows = self._entries.nrows
if self._entries.nrows < self._entries.ncols:
rows = self._entries.ncols # mzd_solve_left requires ncols <= nrows

cdef mzd_t *lhs = mzd_init(rows, self._entries.ncols)
mzd_copy(lhs, self._entries)
cdef mzd_t *rhs = mzd_init(rows, B_entries.ncols)
mzd_copy(rhs, B_entries)

sig_on()
# although it is called mzd_solve_left, it does the same thing as solve_right
ret = mzd_solve_left(lhs, rhs, 0, check)
sig_off()
mzd_free(lhs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this leak the lhs and rhs if user press ctrl+C?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, should I wrap it with a try...finally block or just disable the ability to Ctrl-C?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarity, while _right_kernel_matrix does not enable signal but it can still leak the resulting kernel matrix when Ctrl-C (triggered during self.new_matrix).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly handling it properly is preferable. (But no memory leak is a hard thing to test for…)

cysignals is a bit complicated…

https://cysignals.readthedocs.io/en/latest/interrupt.html#using-sig-on-and-sig-off

So far I don't exactly understand what's the difference between try: sig_on() except KeyboardInterrupt: versus if not sig_on_no_except(): either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for what it's worth, the part I don't understand is sagemath/cysignals#205 .

In any case, I think the following will work:

data = malloc(...)
if not sig_on_no_except():
	mzd_free(lhs)
	mzd_free(rhs)
	cython_check_exception()

ret = mzd_solve_left(...)
sig_off()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have pushed a version where try...finally are used to ensure allocated matrices get freed.


if ret == 0:
# solution is placed in rhs
X = self.new_matrix(nrows=self._entries.ncols, ncols=B_entries.ncols)
rhs.nrows = self._entries.ncols
mzd_copy(X._entries, rhs)
mzd_free(rhs)
return X
else:
mzd_free(rhs)
raise ValueError("matrix equation has no solutions")

def _right_kernel_matrix(self, **kwds):
r"""
Return a pair that includes a matrix of basis vectors
Expand Down
Loading