-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Speed up parent creation for multiplication of square matrices #8096
Comments
Attachment: 8096-new_matrix.patch.gz |
comment:2
I'm not sure that I did this in the right place, but it cuts the time to multiply two 1x1 matrices down to 1/3 of the previous time -- which is still dog slow, but significantly better. As a reference,
and with this patch, I'm getting:
|
comment:3
Replying to @boothby:
But 32.5 µs (with the patch) is much *slower *than |
comment:4
You'll note that the 32.5µs is matrix-by-matrix, whereas the 142ns is element-by-element. Before my patch, the matrix-by-matrix time was 101µs. I'd like the 32.5µs to go away, but I don't know how much of that would be possible, at the moment. |
comment:5
Replying to @boothby:
OK, then it is an improvement. I hope to be able to do some refereeing later today or tomorrow. Anyway, I still wonder why basic matrix operations in Sage tend to be so slow. I mean, do any complicated operations with parents happen behind the scenes? By "slow", I mean "compared with MeatAxe matrices as provided by my cohomology spkg":
|
comment:6
Replying to @simon-king-jena:
This is because they have only been optimized for large dimension and word-sized p. I bet MeatAxe is slower for 1000 x 1000 matrices. |
comment:7
Replying to @robertwb:
You just lost the bet.
When I did some benchmarks two years ago, I was often astonished how MeatAxe outperformed the usual matrices in Sage in basic operations such as computing hash, copying, getting the list of coefficients, changing a coefficient, etc: Hash and copying took seconds for size 10000x10000! I complained, and things did improve: hash and copy is alright. But with the above matrix, loads(dumps(A)) did not terminate after a minute, and ate 90% of my computer's memory. |
Attachment: 8096-zero-matrix.patch.gz apply on top of previous |
comment:8
SimonKing: I'm impressed and depressed. Is MeatAxe's runtime a function of the characteristic? At least one can do
but I agree the current state of matrices are in quite a bit of a mess. Granted, most of this code was written way back before Sage had the quality control it has now, just to get something up and running, and hasn't been touched since. (They're pretty good over Z and Q, which is what I use...) Tom: Your patch looks good, positive review to that. I've posted another patch which provides another 2x speedup. There's still a lot of cruft it goes through, and matrix_space could really stand to be cythonized (there are 3 mandatory Python calls on it every time a matrix is created), but I don't have time to dig through it now. |
comment:9
Replying to @robertwb:
Yes. Over a non-prime field:
The last line took several minutes, and it was not possible to interrupt with Ctrl-c. So, I had to kill the Python process. And with a bigger prime, comparing against linbox:
In other words, for bigger fields, linbox is clearly better than MeatAxe, but the overhead kills it.
Again, there is a huge overhead. But at what point? I mean, the parents of A and B are the same, so, it can't be in the coercion model. And if two matrices have the same parent, then I'd expect that
Which means, a third person (e.g.) should review both Tom's and your patch, right? |
Author: boothby, robertwb |
comment:10
With the patch, I get:
So, there is only a small improvement. But it is an improvement. I currently do sage -testall. If it passes, I'll give it a positive review, and suggest to open two new tickets, one concerning the overhead in multiplying matrices, the other concerning the problem that loads(dumps()) fails on big matrices. |
Work Issues: various doc test errors |
comment:11
There are doctest failures after installing the patches. See http://sage.math.washington.edu/home/SimonKing/logs/8096test.log Some of these failures seem pretty severe. There are unexpected numerical values and recursion depth problems. Do you have an idea how such innocent-looking change can have such a big effect? |
comment:12
Looks to me like the second patch isn't compatible matrix_double_dense -- that's an easy fix. I'm unsure of matrix_2x2, though. I'll check these out after I get some homework done. |
comment:13
Hi Tom! Hopefully your fix will work. This message is mainly to test whether it works with Firefox 3.0.6 on my desktop computer to commit a message starting in wysiwyg mode. |
comment:15
I think it is worth while to revive that ticket! There has been some work done in the past, e.g., on #10763 (already merged) and #11589. I therefore suggest to start with determining the status quo (performance wise). The following is with sage-4.7.rc2 (which contains #10763) and #11589. I provide the timings with sage-4.6.2 in square brackets:
So, part of the issue with small matrices is resolved. The second patch attachment: 8096-new_matrix.patch is actually obsoleted by #10763 and #11589, and the first patch needs to be rebased. However, large matrices are still not nice. We have seen that linbox provides a much faster multiplication. Wouldn't it be a valid approach to use linbox more often? Perhaps make it the default over finite fields? |
comment:16
For the record: Today I slightly improved MeatAxe matrix initialisation (my MeatAxe wrapper relies on a modified/improved version of MeatAxe-2.2.4, and today I added memset). Now, multiplication of two random 2000x2000 matrices over GF(3) takes 1.72 seconds with MeatAxe (it used to be 11 seconds, without memset), compared to 1.65 seconds with _multiply_linbox and 8.34 seconds with plain Sage matrices. I really wonder why linbox isn't used by default. |
Changed work issues from various doc test errors to rebase |
comment:39
Replying to @loefflerd:
No surprise that two-year-old patches don't apply anymore. I am rebasing them now, producing a single patch replacing the two. I am not totally sure, but I think the ideas of the second patch are already in Sage - need to verify it, though. |
comment:40
I have rebased the patches, combining them into one patch. In fact, a part (but not all) of the second patch is not needed anymore, since the creation of the zero matrix has been improved in another ticket. Here is evidence that the patch still does improve the timings for the computation of small matrices - even after we have switched to linbox: Without patch:
With the patch:
I need to run the doctests, though. But needs review. Apply trac8096_speedup_matrix_parent.patch |
This comment has been minimized.
This comment has been minimized.
comment:41
Here is the example from the ticket description. Without the patch:
With the patch:
So, there is an improvement for bigger matrices as well. |
Changed work issues from rebase to none |
comment:43
The patchbot found no doctest errors, but complains that the old patch has introduced white space. Hence, I have updated the patch and hope that it will be fine now. Apply trac8096_speedup_matrix_parent.patch |
Reviewer: PatchBot |
Changed author from boothby, robertwb to Tom Boothby, Robert Bradshaw, Simon King |
comment:44
Curiously the latest version doesn't seem to build on 5.0.beta7: the patch applies, but the modified cython file |
comment:45
Replying to @loefflerd:
Oops. Apparently, while removing white space, I have also removed something else. Sorry, I'll update it in a minute. |
comment:46
Sorry that I did not try to build Sage again after manually editing the patch. I have updated the patch, and now it does build. I leave tests to the patchbot... Apply trac8096_speedup_matrix_parent.patch |
Replaces the previous patches, rebased rel sage-5.0.beta7 |
comment:47
Attachment: trac8096_speedup_matrix_parent.patch.gz Arrgh! There was another trailing whitespace in the patch! It should now be fixed. By the way: Am I entitled to review the patch, even though rebasing the two original patches has not been totally mechanic? I still think that Tom and Robert are the authors, so that I could be reviewer. Agreed? |
comment:48
Patchbot... Apply trac8096_speedup_matrix_parent.patch |
comment:49
I think reviewing patches you have rebased is allowed, as long as there's no change in functionality. |
Changed reviewer from PatchBot to Simon King, David Loeffler |
Merged: sage-5.0.beta9 |
Multiplication of small square matrices is ridiculously slow:
Here's a profile of the 1x1 case:
Apply attachment: trac8096_speedup_matrix_parent.patch
CC: @robertwb @malb
Component: linear algebra
Author: Tom Boothby, Robert Bradshaw, Simon King
Reviewer: Simon King, David Loeffler
Merged: sage-5.0.beta9
Issue created by migration from https://trac.sagemath.org/ticket/8096
The text was updated successfully, but these errors were encountered: