Add AddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matrices#5980
Conversation
OldAnchovyTopping
changed the title
AddMatrixLeft, MultMatrixLeft functions as well as their "right" counterparts
fingolfin
left a comment
fingolfin
left a comment
There was a problem hiding this comment.
Thanks! Some quick comments
|
@OldAnchovyTopping did you see the comments I left you? Just wondering, no pressure -- if you can work some more on this that would be great, but of course you need to know if and when you have time for it. |
|
I've used Codex to update this PR. |
fingolfin
changed the title
AddMatrixLeft, MultMatrixLeft functions as well as their "right" counterpartsAddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matrices
fingolfin
changed the title
AddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matricesAddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matrices, and add some missing MultVectorRight methods
Co-authored-by: Max Horn <max@quendi.de>
Address the outstanding review feedback for AddMatrix and MultMatrix. This adds explicit dimension checks, fixes the 8-bit AddMatrix fast path, completes the right-multiplication specializations, and wires the new manual entries into the matrix object chapter. AI assistance: Codex was used to implement the changes, expand the MatrixObj test coverage, and update the documentation. Co-authored-by: Codex <codex@openai.com>
fingolfin
added
kind: enhancement
fingolfin
changed the title
AddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matrices, and add some missing MultVectorRight methodsAddMatrix, MultMatrix (plus explicit left/right side variants) for in-place modification of matrices
ThomasBreuer
left a comment
ThomasBreuer
left a comment
There was a problem hiding this comment.
A few fixes and some improvements.
|
|
||
| AddMatrix(mat, src); | ||
| AddMatrix(copy, srccopy); | ||
| if not same_entries(mat, copy) then Error("AddMatrix(", scalar, ") failure"); fi; |
There was a problem hiding this comment.
In the current test, scalar plays no role.
| AddMatrix(mat, src); | ||
| AddMatrix(copy, srccopy); | ||
| if not same_entries(mat, copy) then Error("AddMatrix(", scalar, ") failure"); fi; | ||
| if not same_entries(src, srccopy) then Error("AddMatrix source modified"); fi; |
There was a problem hiding this comment.
Both src and srccopy have been involved in an AddMatrix call. If we suspect that this call has modified its second argument then it would be more natural to compare the second argument with a copy that was made before the call and was not used since then.
lib/matobj2.gd
Outdated
| ## | ||
| ## <Description> | ||
| ## These functions multiply the entries of <A>mat</A> by <A>c</A> in-place. | ||
| ## <Ref Oper="MultMatrixRight"/> performs the operation <M>mat \cdot c</M>, |
There was a problem hiding this comment.
GAPDoc syntax
| ## <Ref Oper="MultMatrixRight"/> performs the operation <M>mat \cdot c</M>, | |
| ## <Ref Oper="MultMatrixRight"/> performs the operation <A>mat</A><C>*</C><A>c</A>, |
lib/matobj2.gd
Outdated
| ## <Description> | ||
| ## These functions multiply the entries of <A>mat</A> by <A>c</A> in-place. | ||
| ## <Ref Oper="MultMatrixRight"/> performs the operation <M>mat \cdot c</M>, | ||
| ## whereas <Ref Oper="MultMatrixLeft"/> performs the operation <M>c \cdot mat</M> |
There was a problem hiding this comment.
| ## whereas <Ref Oper="MultMatrixLeft"/> performs the operation <M>c \cdot mat</M> | |
| ## whereas <Ref Oper="MultMatrixLeft"/> performs the operation <A>c</A><C>*</C><A>mat</A> |
lib/matobj2.gd
Outdated
| ## Computes the calculation <M>M + c \cdot N</M> in-place, storing the result in <A>M</A>. | ||
| ## If the optional argument <A>c</A> is omitted, then <A>N</A> is added directly. | ||
| ## If both of the matrices are lists-of-lists, then the program utilises <Ref Oper="AddRowVector"/> | ||
| ## to perform the operation even faster. |
There was a problem hiding this comment.
Is it clear that AddRowVector is faster? (What can AddRowVector do what AddMatrix cannot do?)
There was a problem hiding this comment.
I am not sure I understand the question: AddMatrix and AddRowVector take different argument types a priori? And this is about adding a generic AddMatrix method for lists-of-row-vector" style matrices by using AddRowVector` (for which already several optimized variants exist, e.g. for compressed vectors)
There was a problem hiding this comment.
This documentation is not about a generic method but about the operation itself. Statements about the efficiency of the generic method would belong to a separate documentation of this method.
lib/matobj2.gd
Outdated
| ## and <Ref Oper="MultMatrix"/> is an alias for <Ref Oper="MultMatrixLeft"/>. | ||
| ## In all of these, if the matrix <A>mat</A> is a lists-of-lists, then the program | ||
| ## utilises the fast in-place operations <Ref Oper="MultVectorRight"/> and | ||
| ## <Ref Oper="MultVectorLeft"/> to perform the operation even faster. |
There was a problem hiding this comment.
Is it clear that the MultVector operations are faster? (What can they do what MultMatrix cannot do?)
Refine the AddMatrix and MultMatrix documentation in response to review feedback by using GAPDoc-safe infix markup and describing the row-wise delegation without claiming a speedup. Also strengthen the whole-matrix regression helper so it checks the AddMatrix source argument against an untouched pre-call snapshot. AI assistance: Codex was used to apply the review-driven test and documentation updates. Co-authored-by: Codex <codex@openai.com>
3 participants
To make progress on issue #5952 I implemented the operations of summing and multiplying matrices in-place.
Further work
These functions should now be ready to be used in
meatauto.gito speed up those computations (as well as in other other place that might benefit from in-place matrix operations).The functions also do NOT have coverage tests, that should be added sometime later too.