Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IsIntegralRing to return false for rings that are euclidean but not integral by changing IsUniqueFactorizationRing and IsEuclideanRing to not imply IsIntegralRing anymore #5817

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

fingolfin
Copy link
Member

This is an updated version of PR #3976 by @olexandr-konovalov -- I tried to update that PR but could not, so I opened a new one.

This fixes #3975 and closes #3976.

The main change is that I did not drop the implication

InstallTrueMethod(IsEuclideanRing, IsZmodnZObjNonprimeCollection and
    IsWholeFamily and IsRing);

Instead I changed IsUniqueFactorizationRing (and hence IsEuclideanRing) to no longer imply IsIntegral. I also adjusted docstrings.

Just to avoid confusion, I'd like to point out that IsIntegral is a filter, so it never will give "correct" results in all cases -- it doesn't check the mathematical property but rather has a technical meaning. So IsIntegralRing(R) giving true should mean that the ring R is indeed an integral domain (only up to the existence of another bug, of course). While it giving false does not mean there are zero divisors, just that GAP cannot infer / prove that the ring is integral.

In the discussion on PR #3976, @hulpke suggested that if we do the changes that I now did here, then we should perhaps introduce new filters IsEuclideanDomain or IsEuclideanIntegralRing that have the old meaning (i.e. an alias for IsEuclideanRing and IsIntegralRing). In principle I could do that (though we probably should avoid the "Domain" variant -- while I like that name, unfortunately GAP chose to overload the term domain (and also category) with a meaning that differs from what it normally means in math, and I am concerned about this causing confusion). However I am not sure what I'd do with it? So far all methods I looked at and code I tried seem to work fine with the new meaning. So while I am not opposed, my suggestion would be to hold back on adding that until we find a concrete usecase? Of course if @hulpke or someone else already has something specific in mind right now, please let me know!

Alexander Konovalov and others added 3 commits October 14, 2024 23:11
This fixes an off-by-one error causing missing the case
when zero appears in the diagonal in the multiplication table.

Closes gap-system#3975.
Fields are in filter IsUniqueFactorizationRing since GAP 4.8
Hence IsEuclideanRing also does not anymore
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Oct 15, 2024
@fingolfin fingolfin added this to the GAP 4.14.0 milestone Oct 15, 2024
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes resolve the inconsistencies as far as I see.
It would be helpful to add distinguishing examples to the manual section on IsEuclideanRing, for example IsEuclideanRing( Integers mod 6 ); likewise for IsIntegralRing.

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing in that would obviously cause problems. Nor do I dare (and probably nobody else does so) to claim that it resolves all issues.
Let's try it out and if problems arise revisit later.

@fingolfin fingolfin merged commit 94642bb into gap-system:master Oct 24, 2024
33 checks passed
@fingolfin fingolfin deleted the mh/fix-IsIntegralRing branch October 24, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong result in IsIntegralRing
3 participants