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 performance of computing columns of matrices over GF(2) #38152

Merged
merged 3 commits into from
Jun 9, 2024

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Jun 4, 2024

In response to issue #38150 I have adjusted the request for columns over dense matrices over GF(2) by replacing the standard call to columns with a transpose followed by a request of rows.

This results in almost a 1000x speed up for large matrices (for the example in the issue).

Old Implementation

sage: M2 = random_matrix(GF(2), 2, 2)
sage: %time _  = M2.columns()
CPU times: user 4.14 ms, sys: 1.67 ms, total: 5.81 ms
Wall time: 10.9 ms
sage: %time _  = M2.rows()
CPU times: user 109 µs, sys: 130 µs, total: 239 µs
Wall time: 1.39 ms
sage: M2 = random_matrix(GF(2), 200, 200)
sage: %time _  = M2.columns()
CPU times: user 87.9 ms, sys: 1.32 ms, total: 89.2 ms
Wall time: 88.3 ms
sage: %time _  = M2.rows()
CPU times: user 811 µs, sys: 96 µs, total: 907 µs
Wall time: 912 µs
sage: M2 = random_matrix(GF(2), 2000, 2000)
sage: %time _  = M2.columns()
CPU times: user 7.94 s, sys: 9.11 ms, total: 7.95 s
Wall time: 7.97 s
sage: %time _  = M2.rows()
CPU times: user 7.49 ms, sys: 770 µs, total: 8.26 ms
Wall time: 7.97 ms

New Implementation

sage: M2 = random_matrix(GF(2), 2, 2)
sage: %time _  = M2.columns()
CPU times: user 1.01 ms, sys: 261 µs, total: 1.27 ms
Wall time: 3.75 ms
sage: %time _  = M2.rows()
CPU times: user 54 µs, sys: 8 µs, total: 62 µs
Wall time: 65.1 µs
sage: M2 = random_matrix(GF(2), 200, 200)
sage: %time _  = M2.columns()
CPU times: user 1.09 ms, sys: 40 µs, total: 1.13 ms
Wall time: 1.13 ms
sage: %time _  = M2.rows()
CPU times: user 712 µs, sys: 15 µs, total: 727 µs
Wall time: 732 µs
sage: M2 = random_matrix(GF(2), 2000, 2000)
sage: %time _  = M2.columns()
CPU times: user 9.07 ms, sys: 746 µs, total: 9.82 ms
Wall time: 9.54 ms
sage: %time _  = M2.rows()
CPU times: user 7.82 ms, sys: 687 µs, total: 8.51 ms
Wall time: 8.18 ms

Fixes #38150

Copy link

github-actions bot commented Jun 4, 2024

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

Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

Just small style changes, then it should be good. Thanks for the easy but much needed improvement!

src/sage/matrix/matrix_mod2_dense.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix_mod2_dense.pyx Outdated Show resolved Hide resolved
src/sage/matrix/matrix_mod2_dense.pyx Outdated Show resolved Hide resolved
Add reviewer suggestions

Co-authored-by: grhkm21 <83517584+grhkm21@users.noreply.github.com>
@GiacomoPope
Copy link
Contributor Author

Thanks for the tips -- I was lazy and copy pasted old columns code, so these changes could be applied elsewhere too but that's for another day!

@GiacomoPope GiacomoPope requested a review from grhkm21 June 6, 2024 16:46
Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

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

good enough. I think the entire file can be refactored later lol

@GiacomoPope
Copy link
Contributor Author

Haha yeah one problem at a time I guess

@vbraun vbraun merged commit ec43912 into sagemath:develop Jun 9, 2024
22 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 9, 2024
@GiacomoPope GiacomoPope deleted the faster_columns branch August 21, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requesting columns of matrices over GF(2) is very slow
5 participants