Wrong result in IsIntegralRing

[olexandr-konovalov]

Reported by @grahamknockillaree:

gap> R:=SmallRing(4,3);;
gap> IsIntegralRing(R);
true
gap> ShowMultiplicationTable(R);
*   | 0*a a   2*a -a
----+----------------
0*a | 0*a 0*a 0*a 0*a
a   | 0*a a   2*a -a
2*a | 0*a 2*a 0*a 2*a
-a  | 0*a -a  2*a a

The problem is in the line

gap/lib/ring.gi

Line 704 in 86c4135

for k in [i+1..Length(elms)] do

which should be

for k  in [i..Length(elms)]

to check the diagonal as well.

Thank you for reporting it - I will make a PR tomorrow.

[Stefan-Kohl]

In that example, the zeros don't appear on the diagonal:

gap> IsIntegralRing(Integers mod 6);
true

[olexandr-konovalov]

Thanks for another example @Stefan-Kohl - will look tomorrow, and will use this opportunity to create a proper test for IsIntegralRing. It shown zero coverage at https://codecov.io/gh/alex-konovalov/gap/src/master/lib/ring.gi.

[olexandr-konovalov]

Aha, the reason for Integers mod 6 is different: the answer is triggered by this method:

InstallTrueMethod( IsIntegralRing,
    IsUniqueFactorizationRing and IsNonTrivial );

[fingolfin]

In the Integers mod 6 example, this comes from the implication:

InstallTrueMethod( IsIntegralRing,
    IsUniqueFactorizationRing and IsNonTrivial );

This is also a regression from GAP 4.10. (Since I mostly touched the zmodnz code, most likely this will turn out to be my fault sigh).

So let's bisect it: Create file IntegralRing.tst with content

gap> IsIntegralRing(Integers mod 6);
false

Now start the bisect:

cp etc/bisect.sh /tmp
git bisect bad master
git bisect good stable-4.10
git bisect run /tmp/bisect.sh IntegralRing.tst

Result after a few minutes: 0f9c104 is the first commit (of course as predicted by me, yay :-). Now gotta see what's wrong with it...

[grahamknockillaree]

A related issue is

gap> ShowMultiplicationTable(SmallRing(5,2));

*   | 0*a a   2*a 3*a -a 
----+--------------------
0*a | 0*a 0*a 0*a 0*a 0*a
a   | 0*a a   2*a 3*a -a 
2*a | 0*a 2*a -a  a   3*a
3*a | 0*a 3*a a   -a  2*a
-a  | 0*a -a  3*a 2*a a  

gap> IsField(SmallRing(5,2));
false

IsField is a filter, so I guess this is not a bug. But it could be confusing to a user.

[olexandr-konovalov]

@grahamknockillaree yes, this isn't a bug (one could get a similar confusion with IsGroup for objects that mathematically are groups, but are not constructed as groups in GAP).

[wilfwilson]

The following method introduced in 0f9c104 (the commit @fingolfin bisected the problem to) is inconsistent with GAP:

InstallTrueMethod(IsEuclideanRing, IsZmodnZObjNonprimeCollection and
     IsWholeFamily and IsRing);

In particular, the GAP documentation for IsEuclideanRing begins with:

A ring R is called a Euclidean ring if it is an integral ring and...

Therefore, as things are documented, IsEuclideanRing should return false for Z/nZ with composite n, not true. But as @alex-konovalov seems to have found when writing commit olexandr-konovalov@24b10d2, simply removing this TrueMethod causes GAP to not load.

In more detail, the result of removing the TrueMethod is that this method fails to install:

#############################################################################
##
#M  EuclideanDegree( <R>, <n> )
##
##  For an overview on the theory of euclidean rings which are not domains,
##  see Pierre Samuel, "About Euclidean rings", J. Algebra, 1971 vol. 19 pp. 282-301.
##  http://dx.doi.org/10.1016/0021-8693(71)90110-4

InstallMethod( EuclideanDegree,
    "for Z/nZ and an element in Z/nZ",
    IsCollsElms,
    [ IsZmodnZObjNonprimeCollection and IsWholeFamily and IsRing, IsZmodnZObj and IsModulusRep ],
    function ( R, n )
      return GcdInt( n![1], Characteristic( n ) );
    end );

because there is no longer a matching declaration. This is because EuclideanDegree is declared for [ IsEuclideanRing, IsRingElement ], but IsZmodnZObjNonprimeCollection and IsWholeFamily and IsRing no longer implies IsEuclideanRing. Indeed, the documentation for EuclideanDegree says:

The ring R must be a Euclidean ring

Now, I understand from the helpful message from @fingolfin that his new methods use the notion of Euclidean from http://dx.doi.org/10.1016/0021-8693(71)90110-4, which is more general.

Therefore, either this broader notion of Euclidean needs a different name in GAP, or we need to agree to change the behaviour and documentation of GAP to use this the broader notion.

[wilfwilson]

Sorry, I missed the linked pull request #3976!