Skip to content

refuse to compare two ideals that we don't know how to compare #41040

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 27, 2025
Merged

refuse to compare two ideals that we don't know how to compare #41040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c81110f
comparing ideals ≠ comparing their generators
yyyyx4 Aug 4, 2024
50efa21
I 𝑡ℎ𝑖𝑛𝑘 this piece of code really wanted to compare the ideal generators
yyyyx4 Oct 13, 2025
c291dcb
add test for #37409
yyyyx4 Oct 13, 2025
2c8bed3
catch AttributeError to fix doctest failure in schemes/generic/scheme.py
yyyyx4 Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 62 additions & 29 deletions src/sage/rings/ideal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from sage.categories.rings import Rings
from sage.categories.fields import Fields
from sage.structure.element import MonoidElement
from sage.structure.richcmp import rich_to_bool, richcmp
from sage.structure.richcmp import rich_to_bool, richcmp, op_NE
from sage.structure.sequence import Sequence


Expand Down Expand Up @@ -352,7 +352,7 @@ def random_element(self, *args, **kwds):

def _richcmp_(self, other, op):
"""
Compare the generators of two ideals.
Compare two ideals with respect to set inclusion.
Copy link
Contributor

@user202729 user202729 Oct 14, 2025

Choose a reason for hiding this comment

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

This is only a partial order... right? cf. how are ZZ[x,y] currently compared?

Copy link
Member Author

@yyyyx4 yyyyx4 Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. As far as I can tell from a quick experiment, ideals of ℤ[x,y] are compared by set inclusion, while ideals of ℤ[x] are compared by generators. This patch renders the behaviour uniform in that regard. (It is possible that I missed some other rings that compare ideals in a non-mathematical way.)

Copy link
Contributor

@user202729 user202729 Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

wait, seriously.

sage: R.ideal(x) < R.ideal(y)
False
sage: R.ideal(x) > R.ideal(y)
False

which means…

sage: sorted([R.ideal(x), R.ideal(y)])
[Ideal (x) of Multivariate Polynomial Ring in x, y over Integer Ring,
 Ideal (y) of Multivariate Polynomial Ring in x, y over Integer Ring]
sage: sorted([R.ideal(y), R.ideal(x)])
[Ideal (y) of Multivariate Polynomial Ring in x, y over Integer Ring,
 Ideal (x) of Multivariate Polynomial Ring in x, y over Integer Ring]

at least with the previous behavior, sorting is order-invariant.

actually…

sage: sorted([frozenset({1}), frozenset({2})])
[frozenset({1}), frozenset({2})]
sage: sorted([frozenset({2}), frozenset({1})])
[frozenset({2}), frozenset({1})]

there is a precedent. Maybe it's actually fine.

sage: frozenset({frozenset({2}), frozenset({1})}) == frozenset({frozenset({1}), frozenset({2})})
True

most Python containers are hash-based instead of order-based so they're unaffected by non-total-ordering. (doctest output checker may suffer however)

Copy link
Member Author

@yyyyx4 yyyyx4 Oct 14, 2025

Choose a reason for hiding this comment

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

Yup. Even the comparison between standard Python sets is only partial. Related comments: #37409 (comment), #35546 (comment).

Copy link
Member

@dimpase dimpase Oct 26, 2025

Choose a reason for hiding this comment

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

what is "comparison by generators" for PIDs? Shouldn't one rather check for the mathematically correct "gen1 in (gen2) and gen2 in (gen1)" ?

Copy link
Member Author

@yyyyx4 yyyyx4 Oct 26, 2025

Choose a reason for hiding this comment

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

As far as I can tell, all PIDs that I could immediately think of are doing it right, i.e., perform a mathematical comparison. In some PIDs (such as ZZ, QQ['x']) the generators are normalized (non-negative resp. monic), hence simply comparing the generators would also work.

Copy link
Member

@dimpase dimpase Oct 26, 2025

Choose a reason for hiding this comment

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

hmm, gens of (f) for f in GF(q)[x] normalised too? is monic enough?

Copy link
Member Author

@yyyyx4 yyyyx4 Oct 26, 2025

Choose a reason for hiding this comment

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

They appear to be monic over GFs as well. (In a PID, the generator of any ideal is unique up to scaling by units, so monic is indeed enough to get a canonical generator for the ideal.)

In any case, I'm not sure I understand what your questions are about: Are you worried that we're still not comparing ideals correctly after this patch? Or are you suggesting that it could be done in a simpler/faster/better way for PIDs?


INPUT:

Expand All @@ -362,15 +362,39 @@ def _richcmp_(self, other, op):

EXAMPLES::

sage: R = ZZ; I = ZZ*2; J = ZZ*(-2)
sage: R = ZZ
sage: I = ZZ*2
sage: J = ZZ*(-2)
sage: I == J
True

TESTS:

Check that the example from :issue:`37409` raises an error
rather than silently returning an incorrect result::

sage: R.<x> = ZZ[]
sage: I = R.ideal(1-2*x,2)
sage: I.is_trivial() # not implemented -- see #37409
True
sage: I.is_trivial()
Traceback (most recent call last):
...
NotImplementedError: ideal comparison in Univariate Polynomial Ring in x over Integer Ring is not implemented
"""
if self.is_zero():
return rich_to_bool(op, other.is_zero() - 1)
if other.is_zero():
return rich_to_bool(op, 1) # self.is_zero() is already False
S = set(self.gens())
T = set(other.gens())
if S == T:
return rich_to_bool(op, 0)
return richcmp(self.gens(), other.gens(), op)
if S < T:
return rich_to_bool(op, -1)
if S > T:
return rich_to_bool(op, +1)
raise NotImplementedError(f'ideal comparison in {self.ring()} is not implemented')

def __contains__(self, x):
"""
Expand Down Expand Up @@ -1383,19 +1407,38 @@ def __hash__(self):

def _richcmp_(self, other, op):
"""
Compare the two ideals.
Compare two ideals with respect to set inclusion.

EXAMPLES:
EXAMPLES::

sage: I = 5 * ZZ
sage: J = 7 * ZZ
sage: I == J
False
sage: I < J
False
sage: I > J
False
sage: I <= J
False
sage: I >= J
False
sage: I != J
True

Comparison with non-principal ideal::

sage: R.<x> = ZZ[]
sage: I = R.ideal([x^3 + 4*x - 1, x + 6])
sage: J = [x^2] * R
sage: J = [x + 6] * R
sage: I > J # indirect doctest
True
sage: J < I # indirect doctest
True
sage: I < J # indirect doctest
False
sage: J > I # indirect doctest
False

Between two principal ideals::

Expand All @@ -1408,30 +1451,20 @@ def _richcmp_(self, other, op):
True
sage: I3 = P.ideal(x)
sage: I > I3
True
False
"""
if not isinstance(other, Ideal_generic):
other = self.ring().ideal(other)

try:
if not other.is_principal():
return rich_to_bool(op, -1)
except NotImplementedError:
# If we do not know if the other is principal or not, then we
# fallback to the generic implementation
return Ideal_generic._richcmp_(self, other, op)

if self.is_zero():
if not other.is_zero():
return rich_to_bool(op, -1)
return rich_to_bool(op, 0)

# is other.gen() / self.gen() a unit in the base ring?
g0 = other.gen()
g1 = self.gen()
if g0.divides(g1) and g1.divides(g0):
return rich_to_bool(op, 0)
return rich_to_bool(op, 1)
d1 = self.divides(other)
d2 = other.divides(self)
if d1 or d2:
return rich_to_bool(op, d1 - d2)
return op == op_NE
except (NotImplementedError, AttributeError):
# If we do not know if the other is principal or not,
# then we fall back to the generic implementation
pass

return Ideal_generic._richcmp_(self, other, op)

def divides(self, other):
"""
Expand Down
2 changes: 1 addition & 1 deletion src/sage/rings/polynomial/symmetric_ideal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ def symmetrisation(self, N=None, tailreduce=False, report=None, use_full_group=F
from sage.rings.polynomial.symmetric_reduction import SymmetricReductionStrategy
RStrat = SymmetricReductionStrategy(self.ring(), OUT.gens(),
tailreduce=tailreduce)
while newOUT != OUT:
while newOUT.gens() != OUT.gens():
OUT = newOUT
PermutedGens = list(OUT.gens())
if report is not None:
Expand Down
Loading