Skip to content

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Oct 13, 2025

See #37409: Most users seem to expect ideals to compare equal if and only if they are mathematically equal. The current behaviour alternates between mathematical comparison and "compare generators" comparison, depending on the base ring, which is (1) internally inconsistent and (2) unintuitive to most users.

In this patch I propose to fail by default in the ._richcmp_() method for generic ideals, except for trivial cases in which equality can easily be decided: If the parent ring does not implement a mathematically meaningful comparison operation for ideals, it should fail in a noticeable way, rather than silently falling back to a much weaker notion of equality.

@yyyyx4 yyyyx4 mentioned this pull request Oct 13, 2025
2 tasks
Copy link

github-actions bot commented Oct 13, 2025

Documentation preview for this PR (built with commit 2c8bed3; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

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

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

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

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

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).

@user202729
Copy link
Contributor

some other _richcmp_ returns NotImplemented (object) instead of raise. E.g. rings/ring_extension_morphism.pyx

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 14, 2025

As far as I can tell, in this context, NotImplemented is just another way of saying False — which is not what we want.

@cxzhong
Copy link
Contributor

cxzhong commented Oct 18, 2025

Yes. Two ideals should be compared with respect to the set inclusion. I think it is naturally. You can not compare two ideals with like (x) and (y) in ZZ[x,y]. In that case, we should not return false. We should just throw an error said they are not comparable.

But I think we may need singular library to decide a>b false and b>a false. To raise a error.

@cxzhong cxzhong requested review from dimpase and mantepse October 19, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants