Add more exhaustive mutating op tests to ring conformance test

[lgoettgens]

Resolves #1814.

I open to suggestions for the function names. What I already iterated through:

• Just dispatch (as in the top part of Add ring conformance tests for more unsafe / in place operators #1814 (comment)) is not enough as there are mutating ops with different call semantics but the same number of "default" args.
• Next, I tried something like test_mutating_op_3_2 for the add! test as that may take 3 or 2 arguments. But this was confusing me very fast, and doesn't seems to be extensible to addmul! in a straightforward way.
• Instead, I now added the name of one function that can be tested using this. This then conveys the meaning that such a function can test anything with the same call semantics.

cc @fingolfin

open TODOs:

• decide on function names
• decide which mutating functions can be expected to work in which test_*_interface function
• fix all failures reported by CI

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.12%. Comparing base (6be27f6) to head (8368dc4).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1816      +/-   ##
==========================================
+ Coverage   88.03%   88.12%   +0.09%     
==========================================
  Files         119      119              
  Lines       29982    29982              
==========================================
+ Hits        26396    26423      +27     
+ Misses       3586     3559      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fingolfin]

Function names are fine by me.

[fingolfin]

Thank you, I like it!

[fingolfin]

In practice this won't always work; think of divexact!(::QQMatrix, ::QQMatrix, ::ZZRingElem)

I am also not sure how useful it is in practice.

Here is my impression (I might be missing something, though): if b and a have the same type, this swapped test is redundant; while if tf they have different types, they might be harmful; but if the caller truly wants to test both orders (e.g. both mul!(::ZZRingElem, ::ZZRingElem, ::Integer) and mul!(::ZZRingElem, ::Integer, ::ZZRingElem) they can just call this function twice?

[lgoettgens]

it is not redundant, as it tests a different combination of aliasing between the arguments of f!

I'd rather add another function for *_like_divexact

[lgoettgens]

What remains here is to decide which functions get tested in which interface test. To be sorted are

• inv!
• divexact!
• div!
• rem!
• mod!
• gcd!
• lcm!

I would say inv! is part of the field interface, div!, rem!, mod!, gcd!, lcm! are part of euclidean ring, divexact is part of ring interface. Does this look correct to you?

[fingolfin]

Sounds about right to me. Except perhaps for inv! one could argue that it works in arbitrary rings as long as you feed it units, and you always have one(R). But other than testing it for one(R) and -one(R) I don't see a good way for testing it there (might do that anyway?). For fields we have a much easier time generating example inputs :-)

[fingolfin]

Suggested change

	if typeof(A) == typeof(B)
	a = deepcopy(A)
	b = deepcopy(B)
	a = f!(a, b, a)
	@test equality(a, f(B, A))
	@test b == B
	end
	if typeof(A) == typeof(B)
	a = deepcopy(A)
	b = deepcopy(B)
	a = f!(a, b, a)
	@test equality(a, f(B, A))
	@test b == B
	end

I don't think this is OK, just because the types are equal doesn't mean you can change the order. Think of divexact(4,2) vs. divexact(2,4).

Instead I would suggest to check f!(b, a, b). And if you do that, then perhaps there is no need for a separate test_mutating_op_like_divexact after all?

[fingolfin]

Thinking some more along this vein, perhaps the arguments to test_mutating_op_like_add (and test_mutating_op_like_divexact if we retain it) should include just two values a, b. The third value really is only needed for storage purposes and presumably we can get that via deepcopy? (although, I am also worrying about relying on deepcopy too much here, but that's a separate concern... anyway zero(parent(a)) might be a better source, dunno)

Next, I was wondering whether we should arrange things so that e.g. b is always the last argument, by using these input combos:

• zero(parent(a)), a, b
• a, a, b
• b, a, b (only if a, b have same parent)
• a, b, b (only if a, b have same parent)
• b, b, b (only if a, b have same parent)
• a, b
• b, b (only if a, b have same parent)

The idea being that this way the "divisor" always stays on the right side. And we are testing the parent of a (so if b has a different parent` we don't mix).

But then we loose the ability to test e.g. add!(::ZZRingElem, ::Integer, ::ZZRingElem) by passing an Integer for a and a ZZRingElem for b, because we implicitly assume the ring being tested is parent(a). We could recover this if we passed the "target ring" R (= the parent of a), and then used these input combos (one may wish to also add a check that least one of a and b has R as its parent):

• zero(R), a, b
• a, b
• a, a, b (only if R == parent(a))
• b, a, b (only if R == parent(b))
• a, b, b (only if R == parent(a) == parent(b))
• b, b, b (only if R == parent(a) == parent(b))
• b, b (only if R == parent(a) == parent(b))

[lgoettgens]

I like this and try to implement all of this now. One little thing: I would not only use zero(R) as the temporary result place, but something else as well, e.g. because for 0 we cannot distinguish mul! and addmull!

[lgoettgens]

I now mostly implemented you suggestions from above, with the difference that I don't do any parent checks and take no "target ring".

1. I want this to work for types that don't implement the complete Ring interface, but only want to test one such function. In my example WeightLatticeElem from Oscar, the elements don't even have a parent.
2. We have plenty of fallbacks. The function add!(z::Int, a::ZZRingElem, b::Int) is perfectly valid. It computes a+b and may use z as storage. But in this case, it just won't use z, but that does not matter for us.

[thofma]

I am wondering. Isn't the parent check kind of important? And if we don't do parent checks, are we really testing the ring interface?

[lgoettgens]

I am wondering. Isn't the parent check kind of important? And if we don't do parent checks, are we really testing the ring interface?

Of course functions like test_Ring_interface should do parent checks. But for the newly introduced helper functions, I would like to expect as less as possible from the tested types. For calling them from the test_Ring_interface, this doesn't change anything. But it enables us to call the helper functions from other parts of our testsuite, e.g. eventually I would like to replace https://github.com/oscar-system/Oscar.jl/blob/78da753c2ff63cb6e0968843d20097fc151b56a7/experimental/LieAlgebras/test/RootSystem-test.jl#L135-L206 by a few calls to these new functions.

[lgoettgens]

Question about rem: In the eucl interface docs (https://nemocas.github.io/AbstractAlgebra.jl/stable/euclidean_interface/), this doesn't get mentioned at all. Our types only partially implement it, and in most cases it gets delegated to mod.
Thus the question: is there any difference? If not, do we really need mod! and rem!?

And for the time being, I remove rem! from the eucl interface test, as it is not mentioned in the docs.

[thofma]

My point was that it makes sense to test that the result of c = add!(a, b) has the same parent as a + b. Because in the near future someone might implement it in the wrong way, someone will open an issue and someone might try to fix the interface tests.

Edit: I am fine to keep it as is, but maybe add a comment that the tests are also for things that don't implement the element/parent paradigm (although we have none of this in AA).

[lgoettgens]

My point was that it makes sense to test that the result of c = add!(a, b) has the same parent as a + b. Because in the near future someone might implement it in the wrong way, someone will open an issue and someone might try to fix the interface tests.

This is not how it currently works. See e.g. the following Nemo example:

julia> F1 = cyclotomic_field(4)[1]
Number field with defining polynomial _$^2 + 1
  over rational field

julia> F2 = cyclotomic_field(5)[1]
Number field with defining polynomial _$^4 + _$^3 + _$^2 + _$ + 1
  over rational field

julia> F3 = cyclotomic_field(7)[1]
Number field with defining polynomial _$^6 + _$^5 + _$^4 + _$^3 + _$^2 + _$ + 1
  over rational field

julia> a = gen(F1)
z_4

julia> b = gen(F2)
z_5

julia> c = gen(F3)
z_7

julia> z = add!(a,b,c)
18*z_4 + 207967168

julia> parent(z)
Number field with defining polynomial _$^2 + 1
  over rational field

julia> parent(b+c)
ERROR: no common overstructure for the arguments found
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] force_op(::Function, ::Val{true}, ::AbsSimpleNumFieldElem, ::Vararg{AbsSimpleNumFieldElem})
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/ohL1e/src/AbstractAlgebra.jl:123
 [3] force_op(::Function, ::AbsSimpleNumFieldElem, ::AbsSimpleNumFieldElem)
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/ohL1e/src/AbstractAlgebra.jl:129
 [4] +(a::AbsSimpleNumFieldElem, b::AbsSimpleNumFieldElem)
   @ Nemo ~/code/julia/Nemo.jl/src/antic/nf_elem.jl:292
 [5] top-level scope
   @ REPL[10]:1

So not only doesn't add! set the correct parent, it even skips the parent equality check on the arguments.

adding something like your suggested test to the interface will require a lot of work in Nemo right now to make the interface tests work there again.

[fingolfin]

Fine by me. Maybe add one comment as suggested.

IMHO we should just get this in ASAP and then we can still iterate over it.

E.g. one thing I'd like to see added are tests for the ad-hoc variants of the unsafe ops. Which then raises the question which cases to test; and whether we can make it so that if Nemo is loaded, also ad-hoc operations involving ZZRingElem can be tested. (One idea here would be to have a global

const adhoc_ring_parents = [ parent(1), parent(UInt(1)), parent(UInt8(1)) ]

and then Nemo could just do

push!(AbstractAlgebra.adhoc_ring_parents, ZZ)

Anyway, that's an idea for perhaps a future PR, not this one.

[fingolfin]

Maybe add a comment why this loops is here? Same in other similar places (via copy&paste)

[lgoettgens]

I'll do that after lunch

[lgoettgens]

Done

[thofma]

Yes, if you put garbage into the add! function, you get garbage out. But the test_elem functions do not produce garbage and we never test the mutating functions with garbage in the first place? I don't quite understand the relevance of the example.

[lgoettgens]

I already bumped the version, so that we can tag a release immediately after merging this, so we can test Nemocas/Nemo.jl#1869 with this available.