Revise == and isequal for ring elements

[fingolfin]

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]

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]

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]

In magma it's 'eq' which throws if apples and pears are compared, and 'cmpeq' which returns false. Both are useful, but I prefer errors in bad comparisons. Julia has some strange ideas there...I think based on narrow(ish) use cases in numeric. Generically, almost always it parents do not match it illustrates flaws in my code. Also dicts with mixed key space and "same" keys in different worlds are going to produce lots of oain

…

On Thu, 10 Oct 2024, 09:31 Tommy Hofmann, ***@***.***> wrote: 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? — Reply to this email directly, view it on GitHub <#1854 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA36CV4TI2E24C7GN5MI7NLZ2YUNTAVCNFSM6AAAAABPVNKDN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBUGI4DANRZGU> . You are receiving this because your review was requested.Message ID: ***@***.***>

[thofma]

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]

The strange is to define == to default to false

…

On Thu, 10 Oct 2024, 09:52 Tommy Hofmann, ***@***.***> wrote: 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) — Reply to this email directly, view it on GitHub <#1854 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA36CV7PT5PXQGJ2WF4S7ZLZ2YW5HAVCNFSM6AAAAABPVNKDN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBUGM2DCOBZGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[lgoettgens]

x-ref JuliaLang/julia#40717

[fingolfin]

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.

[fingolfin]

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

[fingolfin]

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)

[fingolfin]

This fails in a really strange test in test/generic/FreeAssociativeAlgebra-test.jl which boils down to this:

   R, x = ZZ["y"]
   # ...
   for num_vars = 1:5
      var_names = ["x$j" for j in 1:num_vars]
      # ...
      _, varlist = polynomial_ring(QQ, var_names)
      y = varlist[1]
      @test x in [x, y]
      @test x in [y, x]   # <- errors out here
      @test !(x in [y])   # <- also errors out if one deletes the previous one
      @test x in keys(Dict(x => 1))
      @test !(y in keys(Dict(x => 1)))  # this line is fine as it uses isequal
   end

I call this strange because (a) this test has nothing to do with free associative algebras, and (b) it is completely unclear to me what this should test in the first place.

Regarding (a) I think this may be due to copy&paste, because the exact same test is also in test/generic/MPoly-test.jl. But that leaves (b) -- what is the point of this? Perhaps it is/was to test what happens if you compare unrelated ring elements? (A comment explaining the intention of the test would have been helpful).

I am tempted to just delete this in both places, unless someone has an idea what this is about (which then might inform how to modify the copy in FreeAssociativeAlgebra-test.jl

Actually as I type this, I wonder: perhaps the mistake is that it does polynomial_ring(QQ, var_names) and the real intention was to do polynomial_ring(R, var_names) -- then this would be about comparing polynomials / free assoc. algebra elements with "coefficients" (where the "coefficients" in this case are polynomials from R) ??

[lgoettgens]

Actually as I type this, I wonder: perhaps the mistake is that it does polynomial_ring(QQ, var_names) and the real intention was to do polynomial_ring(R, var_names) -- then this would be about comparing polynomials / free assoc. algebra elements with "coefficients" (where the "coefficients" in this case are polynomials from R) ??

This is exactly what I thought this did until I re-checked the code again. This would make sense to me, and could be some non-trivial issue with the change of ==.
We should definitely have some test somewhere that tests that coefficients can be compared to polynomials, even in the case that the coefficients themselves are polynomials

[fingolfin]

Let's look into retaining this, and documenting this promotion system (and then perhaps we can also use it to simplify some other code which does ad-hoc stuff right now)

[thofma]

I found what I vaguely remembered. For some reason we made isequal do something different to == for series. This is explained here:

AbstractAlgebra.jl/docs/src/series.md

Lines 293 to 300 in 983f6ec

	Sometimes it is necessary to compare power series not just for arithmetic
	equality, as above, but to see if they have precisely the same precision and
	terms. For this purpose we introduce the `isequal` function.
	For example, if $f = x^2 + O(x^7)$ and $g = x^2 + O(x^8)$ and $h = 0 + O(x^2)$
	then $f == g$, $f == h$ and $g == h$, but `isequal(f, g)`, `isequal(f, h)` and
	`isequal(g, h)` would all return `false`. However, if $k = x^2 + O(x^7)$ then
	`isequal(f, k)` would return `true`.

Unfortunately, this makes the idea of having isequal be the non-throwing version == a bit more difficult to implement. On the other hand, the series have some other issues and I would be happy to rename isequal for them.