booktest: add error checking for non-jlcon files also on master

[benlorenz]

from #4205

[benlorenz]

This uncovered a new error in test/book/cornerstones/number-theory/galoismod_1.jlcon:

julia> for i in 1:8, j in 1:number_of_small_groups(i)
         G = small_group(i, j)
         if is_abelian(G)
           continue
         end
         QG = QQ[G]
         ZG = integral_group_ring(QG)
         println(i, " ", j, " ", describe(G), ": ", locally_free_class_group(ZG))
       end
6 1 S3: Z/1
ERROR: ArgumentError: G1 and G2 are not compatible
Stacktrace:
  [1] #_check_compatible#1398
    @ /home/datastore/lorenz/software/julia/Oscar.jl/src/Groups/types.jl:683 [inlined]
  [2] _check_compatible
    @ /home/datastore/lorenz/software/julia/Oscar.jl/src/Groups/types.jl:681 [inlined]
  [3] ==
    @ /home/datastore/lorenz/software/julia/Oscar.jl/src/Groups/GAPGroups.jl:376 [inlined]
  [4] isequal
    @ ./operators.jl:134 [inlined]
  [5] _isequal (repeats 2 times)
    @ ./tuple.jl:536 [inlined]
  [6] isequal(t1::Tuple{QQField, PcGroup, typeof(*)}, t2::Tuple{QQField, PcGroup, typeof(*)})
    @ Base ./tuple.jl:533
  [7] ht_keyindex(h::Dict{Tuple{Ring, Any, Any}, WeakRef}, key::Tuple{QQField, PcGroup, typeof(*)})
    @ Base ./dict.jl:249
  [8] haskey
    @ ./dict.jl:548 [inlined]
  [9] (::AbstractAlgebra.var"#50#51"{Hecke.var"#155#158"{…}, AbstractAlgebra.WeakValueDict{…}, Tuple{…}})()
    @ AbstractAlgebra /home/datastore/lorenz/software/julia/depot/packages/AbstractAlgebra/DRky3/src/WeakValueDict.jl:544
 [10] lock(f::AbstractAlgebra.var"#50#51"{Hecke.var"#155#158"{…}, AbstractAlgebra.WeakValueDict{…}, Tuple{…}}, l::ReentrantLock)
    @ Base ./lock.jl:232
 [11] lock
    @ /home/datastore/lorenz/software/julia/depot/packages/AbstractAlgebra/DRky3/src/WeakValueDict.jl:512 [inlined]
 [12] get!
    @ /home/datastore/lorenz/software/julia/depot/packages/AbstractAlgebra/DRky3/src/WeakValueDict.jl:542 [inlined]
 [13] get_cached!
    @ /home/datastore/lorenz/software/julia/depot/packages/AbstractAlgebra/DRky3/src/AbstractAlgebra.jl:159 [inlined]
 [14] _
    @ /home/datastore/lorenz/software/julia/depot/packages/Hecke/7wI0D/src/AlgAss/Types.jl:156 [inlined]
 [15] GroupAlgebra
    @ /home/datastore/lorenz/software/julia/depot/packages/Hecke/7wI0D/src/AlgAss/Types.jl:155 [inlined]
 [16] #group_algebra#4266
    @ /home/datastore/lorenz/software/julia/depot/packages/Hecke/7wI0D/src/AlgAss/AlgGrp.jl:72 [inlined]
 [17] group_algebra
    @ /home/datastore/lorenz/software/julia/depot/packages/Hecke/7wI0D/src/AlgAss/AlgGrp.jl:71 [inlined]
 [18] getindex(K::QQField, G::PcGroup)
    @ Hecke /home/datastore/lorenz/software/julia/depot/packages/Hecke/7wI0D/src/AlgAss/AlgGrp.jl:93
 [19] top-level scope
    @ ./REPL[2]:6
Some type information was truncated. Use `show(err)` to see complete types.

The error (from #3166) is not in 1.0 which is why this only happens here and not on the other booktest PR.
Unfortunately before the change in this PR this error was silently ignored on master.

cc: @ThomasBreuer

[thofma]

This is the reason why one might want to have a non-throwing isequal.

[ThomasBreuer]

What happens here is that the constructor for the group algebra R[G] from the ring R and the group G and the group operation op caches the already known results in a dictionary with key (R, G, op).
In order to check whether a given key occurs already, the groups in question are compared for equality via ==.
And since #3166, this comparison is restricted to pairs of groups that are compatible in the sense that multiplying their elements makes sense. (The groups need not have the same type.)

The fact that this caching mechanism tries to compare two groups that are completely unrelated seems to contradict @fieker's verdict that such a call of == indicates an error or a misunderstanding on the side of the user, and that the best way to avoid confusion is to throw an exception.
(Or should the comparison of the keys perhaps better check the group objects for identity?)

[thofma]

The groups are compared using isequal, which falls back to ==. The problem would be solved by having isequal being the non-throwing version of ==.

[ThomasBreuer]

Then we can recommend to write all == methods in the form ==(a, b; error::Bool = true), where an exception is thrown or false is returned (depending on error) if a and b are regarded as not comparable, and isequal methods for our objects can look like isequal(a, b) = ==(a, b, error = false)?

[fingolfin]

I am not sure I'd want to recommend that style, it definitely is not what we do in AA and Nemo so far. In any case, we need a solution now, and the simple one is to add isequal methods for GAPGroup and BasicGAPGroupElem that don't include the parent checks.

I've pushed such methods to this PR now, let's see if they help.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.60%. Comparing base (24c8f63) to head (553ca3a).
Report is 11 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4209   +/-   ##
=======================================
  Coverage   84.59%   84.60%           
=======================================
  Files         631      631           
  Lines       84906    84948   +42     
=======================================
+ Hits        71830    71869   +39     
- Misses      13076    13079    +3     

Files with missing lines	Coverage Δ
src/Groups/GAPGroups.jl	94.22% <100.00%> (+0.18%)	⬆️

... and 12 files with indirect coverage changes

[thofma]

Thanks @fingolfin. I think eventually the cache should use === as @ThomasBreuer suggests. I have to check whether we have some IdDict with "weak values" lying around.

[lgoettgens]

I have to check whether we have some IdDict with "weak values" lying around.

Julia Base has WeakKeyDict, AA has WeakKeyIdDict and WeakValueDict. Maybe you need to squash together the latter two to some WeakValueIdDict in AA?

[fingolfin]

Looks good to me now but it is still marked as draft (@benlorenz ?)