Bugfix and test for IsIntegralRing

[olexandr-konovalov]

Two bugfixes and test for IsIntegralRing

The first one fixes an off-by-one error causing missing the case when zero appears in the diagonal in the multiplication table.

The second one removes true method for IsEuclideanRing which was added in commit 0f9c104 and caused IsIntegralRing wrongly return true e.g. for Integers mod 6,
as reported in #3975.

Closes #3975.

[coveralls]

Coverage increased (+0.005%) to 84.336% when pulling e8501d7 on alex-konovalov:fix-IsIntegralRing into 86c4135 on gap-system:master.

[fingolfin]

But according to the documentation for IsUniqueFactorizationRing, this is correct! If we make this change, we also ought to change the documentation.

But I think the true cause is elsewhere. As I commented on #3975, the commit which introduced this regression is 0f9c104 and it adds another implication which might be the root cause:

InstallTrueMethod(IsEuclideanRing, IsZmodnZObjNonprimeCollection and
    IsWholeFamily and IsRing);

Indeed, I overlooked that the documentation for IsEuclideanRing says:

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

Oops. So that implication is the culprit.

That said, I think it is unfortunate that we define euclidean rings as being integral. So right now I think the quick fix would be to remove the implication I added, but the better fix would be to change the documentation and implications for IsEuclideanRing.

I now have a meeting but can look at this later.

[olexandr-konovalov]

Oh, no, quick fix does not work: just removing the implication causes this:

Error, required filters [ "IsListOrCollection", "IsCollection", "IsFinite", "HasIsFinite", "IsWholeFamily", "HasIsWholeFamily", 
  "IsDuplicateFree", "HasIsDuplicateFree", "IsExtAElement", "CategoryCollections(IsExtAElement)", 
  "CategoryCollections(IsNearAdditiveElement)", "CategoryCollections(IsNearAdditiveElementWithZero)", 
  "CategoryCollections(IsNearAdditiveElementWithInverse)", "CategoryCollections(IsAdditiveElement)", 
  "IsExtLElement", "CategoryCollections(IsExtLElement)", "IsExtRElement", "CategoryCollections(IsExtRElement)", 
  "CategoryCollections(IsMultiplicativeElement)", "CategoryCollections(IsMultiplicativeElementWithOne)", 
  "CategoryCollections(IsMultiplicativeElementWithInverse)", "CategoryCollections(IsAssociativeElement)", 
  "CategoryCollections(IsAdditivelyCommutativeElement)", "CategoryCollections(IsCommutativeElement)", 
  "IsGeneralizedDomain", "IsMagma", "IsAssociative", "HasIsAssociative", "IsCommutative", "HasIsCommutative", 
  "IsGeneratorsOfSemigroup", "HasIsGeneratorsOfSemigroup", "IsNearAdditiveMagma", "IsNearAdditiveMagmaWithZero", 
  "IsNearAdditiveGroup", "IsAdditivelyCommutative", "HasIsAdditivelyCommutative", "IsLDistributive", 
  "HasIsLDistributive", "IsRDistributive", "HasIsRDistributive", "CategoryCollections(IsZmodnZObjNonprime)" ]
for 1st argument do not match a declaration of EuclideanDegree called from
<<compiled GAP code>> from GAPROOT/lib/oper1.g:338 in function InstallMethod called from

[hulpke]

As far as I can see, IsIntegralRing is what is usually called integral domain (but domain has a technical meaning in GAP). I thus do not see the problem with Euclidean rings being defined to be integral, respectively why should Z/6Z be Euclidean?

[fingolfin]

As @fieker explained below, I'd turn the question around: I see no reason why one should restrict "euclidean rings" to be integral, this is arbitrary.

[fieker]

On Wed, Apr 22, 2020 at 01:19:08PM -0700, Alexander Hulpke wrote: @hulpke commented on this pull request. > @@ -623,8 +623,6 @@ InstallSubsetMaintenance( IsIntegralRing, InstallTrueMethod( IsIntegralRing, IsRing and IsMagmaWithInversesIfNonzero and IsNonTrivial ); -InstallTrueMethod( IsIntegralRing, - IsUniqueFactorizationRing and IsNonTrivial ); As far as I can see, `IsIntegralRing` is what is usually called integral domain (but domain has a technical meaning in GAP). I thus do not see the problem with Euclidean rings being defined to be integral, respectively why should Z/6Z be Euclidean?

Because Z/6Z has a Euclidean function and a Euclidean division. You can do HNF, SNF, ... over Z/nZ since it is Euclidean. Its a matter of your taste weather you allow zero divisors in euclidean rings or not. Some make the distinctionbetween Euclidean Rings and Euclidean Domains for that purpose... Along the same lines: all ideals are principal, so it might be a principal ideal ring...

…

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #3976 (comment)

[hulpke]

@fieker Thanks. I think the problematic implication (and definition implying integral) comes from the textbook definition of Euclidean Domain.

I suggest we change the definition of IsEuclideanRing and remove these implications, but to avoid confusion also introduce IsEuclideanDomain or IsEuclideanIntegralRing (and ,if we have one of them, also a distinction between PID and PIR) and then can have the implication for the domain case. There might be similar "Domain" assumptions lurking in EuclideanRing routines or implications.