Reindent lib/local.gi and lib/lib_local.gi for readability by fingolfin · Pull Request #29 · gap-packages/LocalNR

fingolfin · 2026-04-29T16:11:37Z

The code in lib/lib_local.gi has no indentation and as a result is rather hard to read. The code in lib/local.gi is a bit better, but in the end also has problems.

So I've asked codex to reformat the code (only whitespace changes), and it did a decent job. (You can ask GitHub to "ignore whitespace changes" in the "Files" view if you'd like to verify it really only changed formatting)

Keep the contents unchanged apart from whitespace and line breaks. Co-authored-by: Codex <codex@openai.com>

codecov · 2026-04-29T16:14:30Z

Codecov Report

❌ Patch coverage is 96.63158% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.81%. Comparing base (c6a04d2) to head (e578596).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/local.gi	96.30%	10 Missing ⚠️
lib/lib_local.gi	97.05%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   99.85%   97.81%   -2.05%     
==========================================
  Files           5        5              
  Lines         709      733      +24     
==========================================
+ Hits          708      717       +9     
- Misses          1       16      +15

Files with missing lines	Coverage Δ
lib/lib_local.gi	`97.15% <97.05%> (-2.85%)`	⬇️
lib/local.gi	`97.05% <96.30%> (-2.64%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

james-d-mitchell · 2026-04-30T09:44:43Z

+        if ForAll( MaximalSubgroups( G ), x -> IsAbelian( x ) ) then
+          return true;
+        else
+          return false;
+        fi;


Suggested change

if ForAll( MaximalSubgroups( G ), x -> IsAbelian( x ) ) then

return true;

else

return false;

fi;

return ForAll( MaximalSubgroups( G ), x -> IsAbelian( x ) );

This PR is doing purely whitespace changes. I filed #32 recommending code changes like the one you suggest here -- but I really don't want to mix this with the code formatting, and I'd prefer if the package authors did these themselves :-)

(Just to be clear: I do agree with all changes suggested by James)

One final remark: should the authors wish to, of course they can apply all these change suggestions here and merge the result, that's totally cool for me; I just want to be the one to do that, nor do I want to deal with any fallout from e.g. typos that crept in from the changes. ;-)

Oh and @james-d-mitchell perhaps also change the x -> IsAbelian( x ) to just IsAbelian?

Yeah fair enough, I just noticed them, and thought I'd point them out, probably not the right place to do it.

So it seems like this PR without further suggested changes should be merged ASAP, and then the authors should work on further code changes, and can revisit all comments in this PR by @james-d-mitchell and @fingolfin and implement them themselves? Or can they commit James' suggestions and then merge the PR keeping several commits?

Here is my advice to minimize work: since a bunch of good change suggestions were left here, it would be a waste to try to replicate them manually. Instead, I suggest to just use the "Add to batch" feature on all of them (or all that you agree with -- though honestly, I don't see any where I can think of a reason not to apply 'em), and commit the result. This will add a second commit to this PR. Then, assuming CI passes, merge it all.

Agree. @raemarina @IrynaRaievska let's do this tomorrow, to pave the way to the further code refactoring.

Just to also mention that gaplint will point out issues like this, making them easier to discover

We will revisit this suggestion later separately

james-d-mitchell · 2026-04-30T09:45:22Z

+      if Size( h ) > 0 then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if Size( h ) > 0 then

return true;

else

return false;

fi;

return Size( h ) > 0;

GitHub says "Unable to apply suggestions on deleted lines." - we will do this later separately

james-d-mitchell · 2026-04-30T09:45:39Z

+      if IsSubset( U, V ) then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if IsSubset( U, V ) then

return true;

else

return false;

fi;

return IsSubset( U, V );

TODO for later, same as above

james-d-mitchell · 2026-04-30T09:45:55Z

+      if IsAdditiveGroupHomomorphism( map ) then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if IsAdditiveGroupHomomorphism( map ) then

return true;

else

return false;

fi;

return IsAdditiveGroupHomomorphism( map );

TODO for later, same as above

james-d-mitchell · 2026-04-30T09:46:13Z

+      if IsAbelian( G ) and IsLocalNearRing( R ) and
+         ForAll( elm, y -> IsDistributiveElementOfNearRing( R, y ) )
+      then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if IsAbelian( G ) and IsLocalNearRing( R ) and

ForAll( elm, y -> IsDistributiveElementOfNearRing( R, y ) )

then

return true;

else

return false;

fi;

return IsAbelian( G ) and IsLocalNearRing( R ) and

ForAll( elm, y -> IsDistributiveElementOfNearRing( R, y ) );

TODO for later, same as above

james-d-mitchell · 2026-04-30T09:47:12Z

+      if Size( U ) < Size( R ) then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if Size( U ) < Size( R ) then

return true;

else

return false;

fi;

return Size( U ) < Size( R );

TODO for later, same as above

james-d-mitchell · 2026-04-30T09:47:36Z

+      if IsSubset( Un, F ) and Size( Unique( ListX( F, F, \* ) ) ) = Size( F ) then
+        return true;
+      else
+        return false;
+      fi;


Suggested change

if IsSubset( Un, F ) and Size( Unique( ListX( F, F, \* ) ) ) = Size( F ) then

return true;

else

return false;

fi;

return IsSubset( Un, F ) and Size( Unique( ListX( F, F, \* ) ) ) = Size( F );

TODO for later, same as above

james-d-mitchell · 2026-04-30T09:48:01Z

+      if Size( H ) = Size( S ) then
+        return true;
+      fi;
+      return false;


Suggested change

if Size( H ) = Size( S ) then

return true;

fi;

return false;

return Size( H ) = Size( S );

TODO for later, same as above

fingolfin · 2026-04-30T12:44:42Z

+          d := GroupHomomorphismByImagesNC( G, G, gr, x );
+          Add( hom, d );
+        od;
+        f := function( x, y ) return x * y; end;


This functions seems to be unused and could be removed in a follow-up PR (also f from the list of variables) .

fingolfin · 2026-04-30T12:45:58Z

+        constructor := function( z ) return function( u ) return function( x, y ) return u^( PreImage( z, y ) * PreImage( z, x ) ); end; end; end;
+        B := Filtered( hom, x -> IsInjective( x ) );
+        A := GroupByGenerators( B );
+        k1 := Position( OrbitLengths( A, G ), Size( A ) );
+        Or := OrbitsDomain( A, G )[k1];
+        a := Or[1];
+        g := const( a );
+        Em := MagmaWithOne( hom );
+        ma := MappingByFunction( Em, G, g );
+        mul := constructor( ma )( a );


Suggested change

constructor := function( z ) return function( u ) return function( x, y ) return u^( PreImage( z, y ) * PreImage( z, x ) ); end; end; end;

B := Filtered( hom, x -> IsInjective( x ) );

A := GroupByGenerators( B );

k1 := Position( OrbitLengths( A, G ), Size( A ) );

Or := OrbitsDomain( A, G )[k1];

a := Or[1];

g := const( a );

Em := MagmaWithOne( hom );

ma := MappingByFunction( Em, G, g );

mul := constructor( ma )( a );

constructor := { z, u } -> ( { x, y ) -> u^( PreImage( z, y ) * PreImage( z, x ) ) );

B := Filtered( hom, x -> IsInjective( x ) );

A := GroupByGenerators( B );

k1 := Position( OrbitLengths( A, G ), Size( A ) );

Or := OrbitsDomain( A, G )[k1];

a := Or[1];

g := const( a );

Em := MagmaWithOne( hom );

ma := MappingByFunction( Em, G, g );

mul := constructor( ma, a );

fingolfin · 2026-04-30T12:51:35Z

+      T := [];
+      for x in Or do
+        Add( T, Representative( x ) );
+      od;


Suggested change

T := [];

for x in Or do

Add( T, Representative( x ) );

od;

T := List( Or, Representative );

We will revisit this later separately

fingolfin · 2026-04-30T12:52:33Z

+      od;
+      F := [];
+      repeat
+        M := Filtered( T, x -> Order( x ) = Maximum( List( T, Order ) ) );


This recomputes Maximum( List( T, Order ) ) again and again, i.e., is quadratic in the length of T. Better to compute this value once, in the line before, and use that.

fingolfin · 2026-04-30T12:53:57Z

+        g := [];
+        for a in N do
+          Add( g, AsGroupReductElement( a ) );
+        od;


Suggested change

g := [];

for a in N do

Add( g, AsGroupReductElement( a ) );

od;

g := List( N, AsGroupReductElement );

TODO for later, same as above (timing concerns)

fingolfin · 2026-04-30T12:54:13Z

+      else
+        u := Identity( R );
+        a := AsSemigroup( List( R ) );
+        Print( "Semigroup with identity", " ", u, "\n" );


Suggested change

Print( "Semigroup with identity", " ", u, "\n" );

fingolfin · 2026-04-30T12:54:37Z

+      if Identity( R ) = fail then
+        a := AsSemigroup( List( R ) );
+      else
+        u := Identity( R );


This is unused, and so in fact the case where you have and don't have the identity are behaving exactly the same. Is that intentional?

Apparently yes - this is related to SONATA features

Thanks for James and Max! Co-authored-by: Max Horn <max@quendi.de>

olexandr-konovalov

Thanks to @fingolfin and @james-d-mitchell - we have revised and added to the batch what was possible. Some suggestions were marked by GitHub as not applicable - we will redo them manually. A couple of others need further discussion.

olexandr-konovalov · 2026-05-01T11:52:08Z

+      T := [];
+      for x in Or do
+        Add( T, Representative( x ) );
+      od;


We will revisit this later separately

olexandr-konovalov · 2026-05-01T11:52:29Z

+        if ForAll( MaximalSubgroups( G ), x -> IsAbelian( x ) ) then
+          return true;
+        else
+          return false;
+        fi;


We will revisit this suggestion later separately

olexandr-konovalov · 2026-05-01T11:53:48Z

+      if Size( h ) > 0 then
+        return true;
+      else
+        return false;
+      fi;


GitHub says "Unable to apply suggestions on deleted lines." - we will do this later separately

olexandr-konovalov · 2026-05-01T11:54:18Z

+      if IsSubset( U, V ) then
+        return true;
+      else
+        return false;
+      fi;


TODO for later, same as above

olexandr-konovalov · 2026-05-01T11:54:27Z

+      if IsAdditiveGroupHomomorphism( map ) then
+        return true;
+      else
+        return false;
+      fi;


TODO for later, same as above

olexandr-konovalov · 2026-05-01T11:55:17Z

+        g := [];
+        for a in N do
+          Add( g, AsGroupReductElement( a ) );
+        od;


TODO for later, same as above (timing concerns)

olexandr-konovalov · 2026-05-01T11:58:03Z

+      if Identity( R ) = fail then
+        a := AsSemigroup( List( R ) );
+      else
+        u := Identity( R );


Apparently yes - this is related to SONATA features

olexandr-konovalov · 2026-05-01T11:58:13Z

+      if Size( U ) < Size( R ) then
+        return true;
+      else
+        return false;
+      fi;


TODO for later, same as above

olexandr-konovalov · 2026-05-01T11:58:24Z

+      if IsSubset( Un, F ) and Size( Unique( ListX( F, F, \* ) ) ) = Size( F ) then
+        return true;
+      else
+        return false;
+      fi;


TODO for later, same as above

olexandr-konovalov · 2026-05-01T11:58:31Z

+      if Size( H ) = Size( S ) then
+        return true;
+      fi;
+      return false;


TODO for later, same as above

Reindent lib/local.gi and lib/lib_local.gi for readability

42b3273

Keep the contents unchanged apart from whitespace and line breaks. Co-authored-by: Codex <codex@openai.com>

olexandr-konovalov approved these changes Apr 29, 2026

View reviewed changes

james-d-mitchell reviewed Apr 30, 2026

View reviewed changes

fingolfin commented Apr 30, 2026

View reviewed changes

Comment thread lib/lib_local.gi Outdated

fingolfin commented Apr 30, 2026

View reviewed changes

Apply suggestions from code review

e578596

Thanks for James and Max! Co-authored-by: Max Horn <max@quendi.de>

olexandr-konovalov reviewed May 1, 2026

View reviewed changes

olexandr-konovalov merged commit d86f3bd into master May 1, 2026
8 of 9 checks passed

olexandr-konovalov deleted the mh/indentation branch May 1, 2026 12:02

Conversation

fingolfin commented Apr 29, 2026

Uh oh!

codecov Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov Bot commented Apr 29, 2026 •

edited

Loading