Add ring conformance tests for more unsafe / in place operators

[fingolfin]

Right now test/Rings-conformance-tests.jl:169 has this:

      @testset "Unsafe ring operators" begin
         for i in 1:reps
            a = test_elem(R)::T
            b = test_elem(R)::T
            c = test_elem(R)::T
            A = deepcopy(a)
            B = deepcopy(b)
            C = deepcopy(c)

            x = deepcopy(a)
            @test iszero(zero!(x))

            ab = a*b
            x = R()
            @test mul!(x, a, b) == ab
            x = deepcopy(b)
            @test mul!(x, a, b) == ab
            x = deepcopy(a)
            @test mul!(x, x, b) == ab
            x = deepcopy(b)
            @test mul!(x, a, x) == ab

            ab = a+b
            x = R()
            @test add!(x, a, b) == ab
            x = deepcopy(b)
            @test add!(x, a, b) == ab
            x = deepcopy(a)
            @test add!(x, x, b) == ab
            x = deepcopy(b)
            @test add!(x, a, x) == ab
            x = deepcopy(a)
            @test add!(x, b) == ab

            # isapprox as BigFloat may fuse
            # matrices don't implement addmul!
            #t = R()
            #x = deepcopy(a)
            #@test equality(addmul!(x, b, c, t), a+b*c)
            #x = deepcopy(a)
            #@test equality(addmul!(x, x, c, t), a+a*c)
            #x = deepcopy(a)
            #@test equality(addmul!(x, b, x, t), a+b*a)
            #x = deepcopy(a)
            #@test equality(addmul!(x, x, x, t), a+a*a)

            @test A == a
            @test B == b
            @test C == c
         end
      end

So we only test add!, mul! and addmul!. But not zero!, one!, neg! etc. etc. for which we now also have generic implementations. All of those should be added...

And then the tests could also be more comprehensive: right now we only check the results. But we should also add checks that verify that the inputs are generally not modified unexpectedly.

And we should also check the variants, so e.g. add!(a,b) and add!(a,a) should also be tested.

[lgoettgens]

I started doing something like this in https://github.com/oscar-system/Oscar.jl/pull/4165/files#diff-9f8fc45a5e19d896c2d816f269367d03ffa689cfebd1afdfac0ebce93793e3fb for RootSpaceElems. I think this would be quite easy to adjust for the more general case.

Optimally, this conformance test would be a separate function that can then be called from the ring conformance test function, but other types can call this test as well.

[fingolfin]

@lgoettgens ah yes that looks good, great!

For "other types" I assume you have matrices and perhaps groups in mind (for now) ? Of course they support different subsets. So I guess you are saying the separate function should have a signature roughly like this:

function test_unsafe_ops( f, f!, x1 )
  ...
end
function test_unsafe_ops( f, f!, x1, x2 )
  ...
end
...

Here I am assuming we'd have multiple methods to cover ...

• the 1-arg: zero/one
• 1/2-args: neg/inv/abs/...
• 2/3-args: add/mul/
• 3/4-args: addmul/submul (which is a bit special, fewer arg permutations are valid)

Later, the conformance tests could also test e.g. add! not just with three ring elements as argument, but also with the second or third argument being an Int, possibly even other Integer subtypes. Thought while this code is in AA, we couldn't include ZZRingElem here... and some (but not all!) code will also want to test with QQFieldElem, Rational and possibly yet others. So that suggest that down the road we allow instantiatng the "unary ops test" for different argument combos. Pseudo code:

test_unsafe_ops_ring_interface(myRing)  # just with myRing elements
test_unsafe_ops_ring_interface(myRing, ZZ)  # also ZZRingElem args mixed in
test_unsafe_ops_ring_interface(myRing, QQ)

# perhaps could also be declared in one go:
test_unsafe_ops_ring_interface(myRing, [myRing, ZZ, QQ])

[lgoettgens]

I would like to split the ad-hoc tests into #1817.
It shouldn't be too hard adding them, but I am a bit afraid of all the rings that silently violate the interface and the amount of failures we get by adding a test like that. Thus splitting this off from the (possible) failures in #1816