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

Revise == and isequal for ring elements #1854

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

fingolfin
Copy link
Member

This makes it so that == throws if parents don't match, while isequal does not (just returns false). Does not (yet) adjust docs to explain that.

However retain ad-hoc == methods, were the inputs obviously don't have matching parents, so they don't throw.

Question is: do we want this? I think for polynomial & series rings: yes. But in general?

Note have some code to ad-hoc matrix comparison... So the current behavior is not actually changed for matrices. In particular we get this on master but also with this PR:

julia> a = matrix(ZZ, [1 2 ; 3 4]);

julia> b = matrix(QQ, [1 2 ; 3 4]);

julia> a == b
true

julia> c = matrix(QQ, [1 2 ; ]);

julia> b == c
ERROR: Incompatible matrix spaces in matrix operation

Closes #1800, resolves oscar-system/Oscar.jl#4107.
Does not replace #1853 which acts on a lower level.

@lgoettgens
Copy link
Collaborator

I am always confused about the difference in contracts of == and isequal whenever I read any julia documentation about them. Thus I really don't think it's a good idea if we introduce even different semantics for them here. (I am not even sure if this is different from the julia contract...)

@thofma
Copy link
Member

thofma commented Oct 10, 2024

I like it. (It won't work for x in [y, z] (since this uses == for whatever reasons), but dictionaries seem to work, which is fine for me.)

So the rules would be:

  • == errors if parents are not equal, taken promotion into account,
  • isequal yields false in case == would raise an error.

Right?

@fieker
Copy link
Contributor

fieker commented Oct 10, 2024 via email

@thofma
Copy link
Member

thofma commented Oct 10, 2024

As Claus said, this turns isequal into cmpeq from Magma. Here is the docstring (from https://magma.maths.usyd.edu.au/magma/handbook/text/8):

x cmpeq y : Any, Any -> BoolElt
If x and y are comparable, return whether x equals y. Otherwise, return false. Thus this operator always returns a value and an error never results. It is useful when comparing two objects of completely different types where it is desired that no error should occur. However, it is strongly recommended that eq is usually used to allow Magma to pick up common unintentional type errors.

I would not say that julia has strange ideas. It allows for a difference between isequal and ==, which one can use, but one does not need to use. Nothing is broken if one only sticks to == and forgets that isequal exists.

But I am in favor of turning isequal into cmpeq. For a user it is not useful, but it is very useful for library code when doing argument checking. For example, instead of writing

for x in A
  @req parent(x) == R && x == a # where parent(a) == R
end

I can now just write

@req all(isequal(a), A)

@fieker
Copy link
Contributor

fieker commented Oct 10, 2024 via email

@lgoettgens
Copy link
Collaborator

x-ref JuliaLang/julia#40717

@fingolfin
Copy link
Member Author

To clarify: we already had a bunch of isequal methods behaving like I propose. But not all. And conversely some == methods return false for differing parents, and some threw an exception. So this isn't such a big change, it is more about trying to make it a bit more consistent and have something akin to a rule for these things.

Honestly I am not sure how useful it is to have this more generous isequal, and when I started work on this patch I actually wanted to change them all to throw an exception... but then as I edited things I wondered more and more and finally arrived at this...

BTW there are more things to wonder about: e.g. some rings have separate == and isequal methods just for the sake of recursion, i.e.: for a matrix or a polynomial ring, you need to compare coefficients, and in some cases we arrange it so that our == method recursese to ==, while isequal compares coefficients with isequal. However, this is just in some cases, and not universal... Anyway, I am not interested in addressing that here or myself in general, I just though I should mention this as I noticed it last night.

@@ -900,7 +900,7 @@ function ==(f::ConstPoly{T}, g::ConstPoly{T}) where T <: RingElement
end

function isequal(f::ConstPoly{T}, g::ConstPoly{T}) where T <: RingElement
check_parent(f, g)
parent(f) == parent(g) || return false
Copy link
Member Author

Choose a reason for hiding this comment

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

This got more generous (but it's only example code)

@@ -417,8 +417,7 @@ that power series to different precisions may still be arithmetically
equal to the minimum of the two precisions.
"""
function ==(x::FracElem{T}, y::FracElem{T}) where {T <: RingElem}
b = check_parent(x, y, false)
!b && return false
check_parent(x, y)
Copy link
Member Author

Choose a reason for hiding this comment

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

This got stricter. An alternative would be to allow comparison of x and y with differing parents, but IMHO only if that means arithmetic comparison, i.e.: removing the parent check entirely and just proceeding to the following checks involving comparison of numerators and denominators and all. Returning false just because the parents are different is IMHO the worst of all ways to handle this, and a source of errors.

(To be clear, I prefer the error; the point I am trying to make here is that the current behavior is worse than either of the alternatives)

The default method already delegates to ==
... to delegate to isequal for the data
Since isequal is called for e.g. keys in dictionaries, it
makes sense to allow comparing rings with different parents,
and reporting them as being non-equal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comparison of polynomials from different rings throws inconsistently
4 participants