gap-system
Wrong result in IsIntegralRing
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
https://github.com/gap-system/gap/blob/86c4135df7f1396dc19175b457c2b34581ea1b87/lib/ring.gi#L704
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.
In that example, the zeros don't appear on the diagonal:
gap> IsIntegralRing(Integers mod 6);
true
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.
Aha, the reason for Integers mod 6 is different: the answer is triggered by this method:
InstallTrueMethod( IsIntegralRing,
IsUniqueFactorizationRing and IsNonTrivial );
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: 0f9c10479318a4ae19ac18eb494e22b6fae29527 is the first commit (of course as predicted by me, yay :-). Now gotta see what's wrong with it...
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.
@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).
The following method introduced in https://github.com/gap-system/gap/commit/0f9c10479318a4ae19ac18eb494e22b6fae29527 (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 https://github.com/alex-konovalov/gap/commit/24b10d24065155fae11f5010267379132b4dfd1c, 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.
