Improve Matrix_mod2_dense solve_right performance #38843

maple3142 · 2024-10-23T17:36:19Z

Use M4RI builtin mzd_solve_left function to solve equation.

Currently, using solve_right on large GF(2) matrix is pretty slow because it use a generic solving function in matrix2.pyx, but M4RI provides a mzd_solve_left function that does the same thing.

For example, try running this in sage's IPython REPL:

set_random_seed(12345)
n, m = 20000, 19968
A = random_matrix(GF(2), n, m)
x = random_vector(GF(2), m)
B = A*x
%time sol = A.solve_right(B)
assert A * sol == B

without this PR, it took

CPU times: user 1min 2s, sys: 190 ms, total: 1min 2s
Wall time: 15.2 s

with this PR, it becomes

CPU times: user 12.5 s, sys: 40.8 ms, total: 12.6 s
Wall time: 1.09 s

📝 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.

⌛ Dependencies

Use M4RI builtin `mzd_solve_left` function to solve equation.

github-actions · 2024-10-23T18:37:19Z

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

user202729 · 2024-10-24T03:36:49Z

src/sage/matrix/matrix_mod2_dense.pyx

+        # 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)


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

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

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).

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.

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()

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

user202729 · 2024-10-24T10:07:08Z

src/sage/matrix/matrix_mod2_dense.pyx

+        if self._entries.ncols == 0 or B_entries.ncols == 0:
+            # special case: empty matrix
+            if B != 0:
+                raise ValueError("matrix equation has no solutions")


Surely this is redundant (?) if B has no entries then every entries are 0?

(also even if it can happen you forget to free X here)

There are cases where A have zero cols but B have non zero cols, so it would have a different result depends on whether B is zero. And without this, some sage tests will fail.

As for X, I think it is a Python object so it would be handled by Python's refcount?

user202729 · 2024-10-28T07:56:21Z

Maybe add a few tests?

sage: matrix(GF(2), 2, 0).solve_right(matrix(GF(2), 2, 2))
[]
sage: matrix(GF(2), 2, 0).solve_right(matrix(GF(2), 2, 2)+1)
Traceback…

ValueError: matrix equation has no solutions
sage: matrix(GF(2), 2, 0).solve_right(matrix(GF(2), 2, 2)+1, check=False)
[]
sage: matrix(GF(2), 2, 2).solve_right(matrix(GF(2), 2, 0))
[]

(note that the old code returns empty matrix for the third case, while the new code raises error. Apparently it doesn't break any existing test case, but anyway.)

Strange that m4ri doesn't automatically handle the empty matrix case correctly (mathematically) though. Any idea why?

maple3142 · 2024-11-04T17:48:03Z

Tests for special case added.

Strange that m4ri doesn't automatically handle the empty matrix case correctly (mathematically) though. Any idea why?

TBH, zero size matrix is pretty weird by itself I think, so it is not unusual to not consider that I think. It would probably be better handled by the solve_right in matrix2.py so the underlying function don't need to handle that.

maple3142 · 2024-11-05T01:46:10Z

Not sure what breaks the test using Meson on Python 3.9...
The failing test does not fail locally for me too.

user202729 · 2024-11-05T02:31:33Z

My argument: just as zero is not a weird integer, zero-dimensional vector space (which is just a point) is not a weird space. So a zero-sized matrix is just a linear map from (or to) a zero-dimensional vector space.

The math checks out.

(Of course, programming-wise it is still a corner case.)

The failed test case (commit 9c532c5) is

2024-11-04T17:56:52.3683952Z File "src/sage/categories/simplicial_sets.py", line 460, in sage.categories.simplicial_sets.SimplicialSets.Pointed.ParentMethods.covering_map
2024-11-04T17:56:52.3685700Z Failed example:
2024-11-04T17:56:52.3686384Z     C.domain().face_data()
2024-11-04T17:56:52.3686928Z Expected:
2024-11-04T17:56:52.3687318Z     {(*, ()): None,
2024-11-04T17:56:52.3687752Z      (*, (1,2,3)): None,
2024-11-04T17:56:52.3688211Z      (*, (1,3,2)): None,
2024-11-04T17:56:52.3750815Z      (sigma_1, ()): ((*, (1,2,3)), (*, ())),
2024-11-04T17:56:52.3751444Z      (sigma_1, ()): ((*, ()), (*, ())),
2024-11-04T17:56:52.3752076Z      (sigma_1, (1,2,3)): ((*, (1,3,2)), (*, (1,2,3))),
2024-11-04T17:56:52.3752801Z      (sigma_1, (1,2,3)): ((*, (1,2,3)), (*, (1,2,3))),
2024-11-04T17:56:52.3753508Z      (sigma_1, (1,3,2)): ((*, ()), (*, (1,3,2))),
2024-11-04T17:56:52.3754218Z      (sigma_1, (1,3,2)): ((*, (1,3,2)), (*, (1,3,2)))}
2024-11-04T17:56:52.3754795Z Got:
2024-11-04T17:56:52.3755155Z     {(*, ()): None,
2024-11-04T17:56:52.3755553Z      (*, (1,2,3)): None,
2024-11-04T17:56:52.3755990Z      (*, (1,3,2)): None,
2024-11-04T17:56:52.3756430Z      (sigma_1, ()): ((*, ()), (*, ())),
2024-11-04T17:56:52.3757008Z      (sigma_1, ()): ((*, (1,2,3)), (*, ())),
2024-11-04T17:56:52.3757684Z      (sigma_1, (1,2,3)): ((*, (1,2,3)), (*, (1,2,3))),
2024-11-04T17:56:52.3758355Z      (sigma_1, (1,2,3)): ((*, (1,3,2)), (*, (1,2,3))),
2024-11-04T17:56:52.3759027Z      (sigma_1, (1,3,2)): ((*, (1,3,2)), (*, (1,3,2))),
2024-11-04T17:56:52.3759724Z      (sigma_1, (1,3,2)): ((*, ()), (*, (1,3,2)))}

looks completely unrelated to this change. And the only difference is in the ordering of the keys so… probably it's fine.

On the other hand Python dict is ordered by insertion time, and it looks weird that a dict can have two keys that is exactly equal…? (they're probably not equal but have same representation or something. Worth opening a new issue for that.)

user202729 · 2024-11-08T14:25:42Z

Looks like the failing test is #38940 . Should be unrelated to this change then.

user202729 · 2024-11-08T14:54:00Z

src/sage/matrix/matrix_mod2_dense.pyx

+            sage: matrix(GF(2), 2, 0).solve_right(matrix(GF(2), 2, 2) + 1, check=False) == matrix(GF(2), 0, 2)
+            True
+            sage: matrix(GF(2), 2, 2).solve_right(matrix(GF(2), 2, 0)) == matrix(GF(2), 2, 0)
+            True


Suggested change

True

True

Check that it can be interrupted::

sage: set_random_seed(12345)

sage: n, m = 20000, 19968

sage: A = random_matrix(GF(2), n, m)

sage: x = random_vector(GF(2), m)

sage: B = A*x

sage: alarm(0.5); sol = A.solve_right(B)

Traceback (most recent call last):

...

AlarmInterrupt

Improve Matrix_mod2_dense solve_right performance

b1e08e6

Use M4RI builtin `mzd_solve_left` function to solve equation.

github-actions bot added the s: needs review label Oct 23, 2024

Fix code style

d1d21f8

user202729 reviewed Oct 24, 2024

View reviewed changes

maple3142 added 4 commits October 24, 2024 13:26

Fix Matrix_mod2_dense solve_right signal handling

5a68fae

Fix Matrix_mod2_dense solve_right empty matrix

8f59819

Fix Matrix_mod2_dense solve_right empty matrix

a2486ee

Fix Matrix_mod2_dense solve_right empty matrix

88ca4ca

user202729 reviewed Oct 24, 2024

View reviewed changes

Add special case to Matrix_mod2_dense solve_right

9c532c5

user202729 reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Matrix_mod2_dense solve_right performance #38843

Improve Matrix_mod2_dense solve_right performance #38843

maple3142 commented Oct 23, 2024

github-actions bot commented Oct 23, 2024 •

edited

Loading

user202729 Oct 24, 2024

maple3142 Oct 24, 2024

maple3142 Oct 24, 2024

user202729 Oct 24, 2024

user202729 Oct 24, 2024

maple3142 Oct 24, 2024

user202729 Oct 24, 2024

maple3142 Oct 24, 2024 •

edited

Loading

user202729 commented Oct 28, 2024 •

edited

Loading

maple3142 commented Nov 4, 2024

maple3142 commented Nov 5, 2024 •

edited

Loading

user202729 commented Nov 5, 2024 •

edited

Loading

user202729 commented Nov 8, 2024

user202729 Nov 8, 2024

-            True
+            True
+        Check that it can be interrupted::
+            sage: set_random_seed(12345)
+            sage: n, m = 20000, 19968
+            sage: A = random_matrix(GF(2), n, m)
+            sage: x = random_vector(GF(2), m)
+            sage: B = A*x
+            sage: alarm(0.5); sol = A.solve_right(B)
+            Traceback (most recent call last):
+            ...
+            AlarmInterrupt

Improve Matrix_mod2_dense solve_right performance #38843

Are you sure you want to change the base?

Improve Matrix_mod2_dense solve_right performance #38843

Conversation

maple3142 commented Oct 23, 2024

📝 Checklist

⌛ Dependencies

github-actions bot commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maple3142 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

user202729 commented Oct 28, 2024 • edited Loading

maple3142 commented Nov 4, 2024

maple3142 commented Nov 5, 2024 • edited Loading

user202729 commented Nov 5, 2024 • edited Loading

user202729 commented Nov 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2024 •

edited

Loading

maple3142 Oct 24, 2024 •

edited

Loading

user202729 commented Oct 28, 2024 •

edited

Loading

maple3142 commented Nov 5, 2024 •

edited

Loading

user202729 commented Nov 5, 2024 •

edited

Loading