Add more exhaustive mutating op tests to ring conformance test

[lgoettgens]

Resolves https://github.com/Nemocas/AbstractAlgebra.jl/issues/1814.

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

• Just dispatch (as in the top part of https://github.com/Nemocas/AbstractAlgebra.jl/issues/1814#issuecomment-2384388098) 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:

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

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.12%. Comparing base (6be27f6) to head (8368dc4). Report is 3 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[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 :-)

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

[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 https://github.com/Nemocas/Nemo.jl/pull/1869 with this available.