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

Added AddMatrixLeft, MultMatrixLeft functions as well as their "right" counterparts #5980

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

OldAnchovyTopping
Copy link
Contributor

@OldAnchovyTopping OldAnchovyTopping commented Apr 11, 2025

  • Implemented the AddMatrixLeft/Right and MultMatrixLeft/Right methods.
  • Implemented the method MultVectorRight for lists to support MultMatrixRight method.

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.gi to 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.

@OldAnchovyTopping OldAnchovyTopping changed the title TBAL Added AddMatrixLeft, MultMatrixLeft functions as well as their "right" counterparts Apr 11, 2025
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks! Some quick comments

InstallMethod( AddMatrix, "for a mutable 8bit matrix and an 8bit matrix",
[ Is8BitMatrixRep and IsMutable, Is8BitMatrixRep ],
function( dstmat, srcmat )
local i, j;
Copy link
Member

Choose a reason for hiding this comment

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

The linter is unhappy, this is one of the changes needed to make it happy:

Suggested change
local i, j;
local i;

Comment on lines +1916 to +1917
# Assert(0, NrRows(dstmat) = NrRows(srcmat));
# Assert(0, NrCols(dstmat) = NrCols(srcmat));
Copy link
Member

Choose a reason for hiding this comment

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

These should either be removed, or turned into working code.

Suggested change
# Assert(0, NrRows(dstmat) = NrRows(srcmat));
# Assert(0, NrCols(dstmat) = NrCols(srcmat));

or

Suggested change
# Assert(0, NrRows(dstmat) = NrRows(srcmat));
# Assert(0, NrCols(dstmat) = NrCols(srcmat));
Assert(1, DimensionsMat(dstmat) = DimensionsMat(srcmat));

Comment on lines +1932 to +1933
# Assert(0, NrRows(dstmat) = NrRows(srcmat));
# Assert(0, NrCols(dstmat) = NrCols(srcmat));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Assert(0, NrRows(dstmat) = NrRows(srcmat));
# Assert(0, NrCols(dstmat) = NrCols(srcmat));
Assert(1, DimensionsMat(dstmat) = DimensionsMat(srcmat));

Copy link
Member

Choose a reason for hiding this comment

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

Similar in other places which I did not mark

InstallMethod( AddMatrix, "for a mutable 8bit matrix, an 8bit matrix, and a scalar",
[ Is8BitMatrixRep and IsMutable, Is8BitMatrixRep, IsFFE ],
function( dstmat, srcmat, scalar )
local i, j;
Copy link
Member

Choose a reason for hiding this comment

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

The linter is unhappy, this is one of the changes needed to make it happy:

Suggested change
local i, j;
local i;

## <Returns>nothing</Returns>
##
## <Description>
## <P/>
Copy link
Member

Choose a reason for hiding this comment

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

No need to start with a paragraph break here, I think?

Suggested change
## <P/>


############################################################################
##
## <#GAPDoc Label="AddMatrixRight">
Copy link
Member

Choose a reason for hiding this comment

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

This chunk needs to be included by one of the doc/ref/*.xml files, via <#Include Label="AddMatrixRight">

Otherwise it won't appear in the manual.

Copy link
Member

Choose a reason for hiding this comment

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

Also it should be changed to AddMatrix :-) also below


############################################################################
##
## <#GAPDoc Label="MultMatrix">
Copy link
Member

Choose a reason for hiding this comment

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

This chunk also needs to be added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants