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.
| ## 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.
There was a problem hiding this comment.
Thanks for the explanation, now I understand what you meant, and agree.
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>
…ap-system#5980) AI assistance: Codex was used to implement some of the code changes, expand the MatrixObj test coverage, and update the documentation. Co-authored-by: Max Horn <max@quendi.de> 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.