Partial fix and workaround for #2400

[hulpke]

As requested made this a PR of its own. It avoids creating magmas from group elements that have the wrong order.

Tests for this are already in the tst files (therefore no extra test provided here) but will be triggered only once the new method for SetSize in #2387 has been merged.

[codecov]

Codecov Report

Merging #2429 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2429      +/-   ##
==========================================
- Coverage   73.96%   73.96%   -0.01%     
==========================================
  Files         484      484              
  Lines      245455   245458       +3     
==========================================
+ Hits       181544   181546       +2     
- Misses      63911    63912       +1

Impacted Files	Coverage Δ
lib/addmagma.gi	25.88% <100%> (ø)	⬆️
lib/magma.gi	54.86% <100%> (+0.26%)	⬆️
hpcgap/lib/hpc/stdtasks.g	64.7% <0%> (-0.26%)	⬇️

[fingolfin]

The commit message summary is "FIX: (partial) for #2400 " which is not very helpful when looking at a log (nobody will know what #2400 is by heart). Better to actually summarize what is fixed, e.g.:
Fix two bugs related to empty (sub)magmas

[fingolfin]

If I were to look at this code in a year, I'd be completely confused as to what it's about. Even if I spend the extra effort to use git annotate to find the commit message and look at it, I don't think I'd be much wiser: "... replacing the empty submagma with the one-submagma, if it turns out a group" -- Huh? How can the submagma turn out to be a group, if it is empty? Isn't it a bug that it's marked as a group in the first place? Or is the bug that it's marked as a group"?

In fact, reading up on #2400 again, I am not convinced this change is correct. Rather, it looks like a workaround for a deeper issue. As such, I don't feel comfortable merging it, until somebody explains why it is correct after all (and ideally that explanation then should be part of a comment before this if/fi block).

[hulpke]

Yes, it is a workaround (and I've added a comment to this effect). The problem is that this bug arises in test suites (in examples of considering groups as magmas) and has the potential of holding up other commits, while it is completely unclear to me how one would fix this.

[fingolfin]

I agree with this change.

[fingolfin]

A proper fix for the second issue would be this (as described by @ThomasBreuer on issue #2400):

diff --git a/lib/magma.gd b/lib/magma.gd
index d939bcd62..6577c0921 100644
--- a/lib/magma.gd
+++ b/lib/magma.gd
@@ -130,7 +130,7 @@ DeclareCategory( "IsMagmaWithInverses",
     and IsMultiplicativeElementWithInverseCollection );

 InstallTrueMethod( IsMagmaWithInverses,
-    IsFiniteOrderElementCollection and IsMagma );
+    IsFiniteOrderElementCollection and IsMagmaWithOne );

 #############################################################################
 ##

In theory, that would loose some features, but in practice, this does not seem to matter, at least for the testsuite.

We could also add a comment saying e.g.

# TODO: replace 'IsMagmaWithOne' with 'IsMagma and IsNonEmpty' if it ever becomes available.

I also have a local patch which adds IsNonEmpty and IsInfinite, with suitable implications (and in the future, also the negative implications, although in practice they seem less important/useful). But I think for now, the above patch should be fine, and be much better than a workaround, IMHO -- assuming it does solve the problem you are seeing?