Clean up 8 bit matrix #5260

james-d-mitchell · 2022-12-11T14:23:31Z

This PR further cleans up the code in lib/mat8bit.gi by:

using uniform code formatting
fixing/making uniform the description strings of the methods in the file
making the filters for methods used more uniform
removing some duplicate/unnecessary code

Altogether this removes about 400 lines of code from this file. This is more preparation for tackling #4914, the changes in this PR should make it easier to make the changes necessary for #4914. Some things that weren't completely clear to me (but also didn't seem to cause the tests to fail) were:

There were lots of methods with filters IsList and Is8BitMatrixRep or IsSmallList and Is8BitMatrixRep or IsMatrix and Is8BitMatrixRep. Looking at the code it seems like every object belonging to Is8BitMatrixRep must belong to IsSmallList and IsMatrixObj. So, I removed the unnecessary IsList, IsSmallList, or IsMatrix, wherever that worked. This might be a terrible idea, I'm happy to reverse these changes if so.
There was a fair amount of duplicated code in lib/mat8bit.gi for XMutable, XImmutable, and XSameMutability (where X = One, X = Zero etc), and products of ffe's and Is8BitMatrixRep, which I've removed/replaced. I suppose it's possible that this has some negative impact on performance (an additional function call + method selection), but I couldn't detect any.
I removed as many direct uses of the representation Is8BitMatrixRep as I could, which I suppose might also have some negative performance implications (but again I couldn't detect any). This will make resolving MatrixObj: support Is8BitMatrixRep with zero rows (so 0 x N) #4914 more straightforward.

I'm not particularly attached to any of the changes, and I'm happy to modify this if anyone feels strongly about it.

I'm opening this just now to check that the tests run, I want to add some more tests before taking it any further.

Text for release notes

None

fingolfin · 2022-12-13T12:56:52Z

tst/testinstall/meataxe.tst

 gap> subs:=SMTX.MinimalSubGModules(M2,M5);
-[ < immutable compressed matrix 4x10 over GF(49) > ]
+[ < immutable compressed matrix 4x10 over GF(7^2) > ]


I have not yet had a chance to look at the rest of this PR (it is rather big; the bigger the PR, the longer it takes until someone finds time to review it properly).

But I can already say that this change is problematic, because it would break the test suites and/or manual examples of a bunch of GAP packages. Is it really necessary to make this change to the printing here? If we really want to do that, it would be better to have it in its own, isolated PR, where we then can discuss plans for a migration. Affected packages include:

atlasrep

classicpres

ctbllib

fining

gbnp

orb

semigroups

spinsym

The change isn't necessary, and is easy to reverse, I'm happy to do it. I'm also happy to split this PR up in to smaller chunks if that's desirable.

fingolfin

Thanks James, I really appreciate that you are working on this.

However, I am afraid this PR goes a bit overboard, and has various problems.

I know the itch, but perhaps it's better to suppress the desire to reformat and clean up everything for now....

Alternatively, I would be open to one PR which removes trailing whitespace in all library files, and perhaps also changes tabs to spaces. Then we could set up a .git-blame-ignore-revs file to ignore the changes in that commit for purposes of git blame and similar.

I've left a couple comments to indicate other problems. I am happy to discuss ways how we can efficiently overcome these.

fingolfin · 2022-12-13T12:59:46Z

lib/mat8bit.gi

-        SEMIECHELON_LIST_VEC8BITS_TRANSFORMATIONS);
-
+# TODO: There's a bunch of argument checking in SEMIECHELON_LIST_VEC8BITS that
+# could be done by method selection.


Again this was a note to myself, and not intended for review...

fingolfin · 2022-12-13T13:04:59Z

lib/mat8bit.gi

-    r := m![1];
-    c := LEN_VEC8BIT(m![2]);
+    r := NumberRows(m);
+    c := NumberColumns(m);


It seems the commit mat8bit: fix descript. strings + filters does more than its description suggests?

Yes, again this was a draft.

fingolfin · 2022-12-13T13:06:48Z

lib/mat8bit.gi

@@ -232,7 +226,7 @@ end );

 InstallMethod( \+, "for two 8-bit matrices",
        IsIdenticalObj, [Is8BitMatrixRep,
-                Is8BitMatrixRep], 0,
+                Is8BitMatrixRep], ,


Ooops, , , is a syntax error. I guess a later commit fixes this? Breaks the ability to git bisect, though :-(

fingolfin · 2022-12-13T13:08:24Z

lib/mat8bit.gi

-#############################################################################
-##
-#V  TYPES_MAT8BIT . . . . . . . . prepared types for compressed GF(q) vectors
-##


The comments removed in commit mat8bit: remove vacuous comments may be mostly vacuous, but they are also used throughout the whole GAP library and kernel source, and some may consider them helpful for navigating the source files.

It feels a bit odd to remove them in this one file, as part of this particular PR.

Yes, I wondered about this, but I took inspiration from your recent PR @fingolfin, where you delete some similar vacuous comments: /~https://github.com/gap-system/gap/pull/5256/files, although that was in the kernel and you didn't remove nearly as many as I did. There are files in the library where there are comments like this and others where there aren't, I don't think we can claim that there's much uniformity in the conventions used in the library...

fingolfin · 2022-12-13T13:10:02Z

lib/mat8bit.gi

-    Objectify(TYPE_MAT8BIT(Q_VEC8BIT(m![2]), true),c);
-    return c;
-end );
+# TODO renovate


This commit performs code formatting and inserts comments. Now I wonder if it also performs other changes that I then effectively can't review, because I have no good way to find them other than to go through this huge PR line by line (or go through each commit line by line, essentially covering everything multiple times) :-(

UPDATE: OK, using "Hide whitespace" in the GitHub web view helps a bit for this particular reformatting commit

Again this was a draft and not finished or ready for review, I'm sorry if I wasted your time.

fingolfin · 2022-12-13T13:11:28Z

lib/mat8bit.gi

-        function( arg )
-    local m,qs, v,  q, givenq, q1, LeastCommonPower, lens;
+InstallMethod(\+, "for two 8-bit matrices", IsIdenticalObj,
+[Is8BitMatrixRep, Is8BitMatrixRep], SUM_MAT8BIT_MAT8BIT);


This fixes formatting and a syntax error

fingolfin · 2022-12-13T13:12:28Z

lib/mat8bit.gi

-#        l[1] := 1;
-#        l[2] := v;
-#        SetFilterObj(v,IsLockedRepresentationVector);
-#        Objectify(TYPE_MAT8BIT(Q_VEC8BIT(v), true), l);


Clearly the commit "mat8bit: use uniform code formatting" does quite a bit more than its title suggests

Sure, again this wasn't intended for review and was marked as a "draft"...

fingolfin · 2022-12-13T13:13:20Z

lib/mat8bit.gi

@@ -89,7 +89,7 @@ InstallMethod(Unbind\[\],
 "for a mutable 8-bit matrix rep and pos. int.",
 [IsMutable and Is8BitMatrixRep, IsPosInt],
 function(m, p)
-  if p = 1 or p <> m![1] then
+  if p = 1 or p <> NumberRows(m) then


I approve of this change, but don't understand why it is in the commit mat8bit: remove duplicate/unnecessary code ?

This PR was marked as a draft, it wasn't intended for review (although I'm happy enough to have your comments).

fingolfin · 2022-12-13T13:15:35Z

lib/mat8bit.gi

-  fi;
-  return m * s;
-end);
+[Is8BitMatrixRep, IsFFE], {m, s} -> s * m);


Are we sure this delegation is safe and won't lead to an infinite loop at some point? For other such cases, we carefully define what can delegate to what, to avoid infinite back-and-forth-delegation.

No, no more than there any guarantees about anything. However, this could be refactored differently to avoid this possibility.

fingolfin · 2022-12-13T14:28:26Z

lib/mat8bit.gi

-end);
+mat -> MakeImmutable(AdditiveInverseMutable(mat)));
+
+# TODO is the next method required? Doesn't use the representation at all


It does use the representation in so far as that it "knows" from the representation that the row objects can be "locked" by setting the IsLockedRepresentationVector filter.

Unfortunately your new code is not equivalent to the old and in particular does not meet the requirements expressed in the documentation for AdditiveInverseSameMutability. Specifically: if a matrix is mutable but with immutable rows, then the result of AdditiveInverseSameMutability will again be a mutable matrix with immutable rows... I am not saying this is a great API (indeed MatrixObj does away with this kind of oddity), but as long as that's what our documentation says, we should abide by it.

In fact trying to read up on the rules behind all this also lead to me making PR #5263 😂.

Aha, ok, this was one of my questions, it wasn't clear to me if this was allowed, some of the code in this file also seems to indicate that it's possible for some rows to be mutable and others to be immutable, is that possible? Interestingly, there don't seem to be any tests that expose the behaviour you (and the doc) describe.

james-d-mitchell · 2022-12-13T15:05:23Z

Thanks for the comments @fingolfin, I'll close this and try again in smaller steps...

james-d-mitchell · 2022-12-13T15:48:47Z

Just to comment that I'm sorry if I wasted your time opening this PR @fingolfin, I'm happy to receive your comments, but hadn't intended for it to be reviewed. I did mark the PR as a draft, and I wrote in the original PR text that I opened it:

to check that the tests run

I'm not really sure how to progress this at this point, perhaps we can discuss on Thursday?

james-d-mitchell added 10 commits December 11, 2022 14:05

mat8bit: remove trailing whitespace and tabs

dafa819

mat8bit: fix descript. strings + filters

57549b7

mat8bit: remove redundant args for InstallMethod

7e6a014

mat8bit: remove vacuous comments

f11fbcb

mat8bit: use uniform code formatting

deabe12

mat8bit: fix more description strings

dfd9187

mat8bit: remove duplicate/unnecessary code

f1747e5

mat8bit: avoid using representation where possible

143e396

mat8bit: remove redundant methods

dee4cce

mat8bit: fix comment

f123cac

james-d-mitchell marked this pull request as draft December 11, 2022 15:35

mat8bit: fix manual examples

f4132ef

fingolfin reviewed Dec 13, 2022

View reviewed changes

fingolfin requested changes Dec 13, 2022

View reviewed changes

james-d-mitchell closed this Dec 13, 2022

james-d-mitchell deleted the clean-up-8-bit-matrix branch December 13, 2022 15:16

james-d-mitchell mentioned this pull request Dec 14, 2022

Remove trailing whitespace from all files in lib #5268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up 8 bit matrix #5260

Clean up 8 bit matrix #5260

james-d-mitchell commented Dec 11, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin left a comment

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022 •

edited

Loading

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

fingolfin Dec 13, 2022

james-d-mitchell Dec 13, 2022

james-d-mitchell commented Dec 13, 2022

james-d-mitchell commented Dec 13, 2022

Clean up 8 bit matrix #5260

Clean up 8 bit matrix #5260

Conversation

james-d-mitchell commented Dec 11, 2022

Text for release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-d-mitchell commented Dec 13, 2022

james-d-mitchell commented Dec 13, 2022

james-d-mitchell Dec 13, 2022 •

edited

Loading