Fix two bugs related to empty magmas

[fingolfin]

First, SubadditiveMagma(a,[]) computes an empty magma, but we erroneously set
IsTrivial for it, which implies size 1. This was changed to IsEmpty.

Secondly, there was an implication from "IsFiniteOrderElementCollection and
IsMagma" to IsMagmaWithInverses. But such a collection may be empty, hence the
implication is invalid as given. This PR just removes it (instead of trying to do something clever),
which seems to have no negative consequences -- if anybody is aware of any, please let me know.

Fixes #2400
Closes #2429

[ThomasBreuer]

This is o.k.

My only idea for turning the wrong implication into a correct one would be
InstallTrueMethod( IsMagmaWithInverses, IsFiniteOrderElementCollection and IsMagmaWithOne ),
but probably this is not worth the effort.

[hulpke]

I agree that this will also fix the issue at hand (and indeed then should override #2429)

However before merging we should make sure that we don't end up with bona-fide groups that do not know so.

Would it be possible to temporarily (just for tests) make this an immediate method that will trigger an error for groups? (I would hope it never gets triggered which would be proof that this indeed harmless.)

If that was the case one should add to have this flag set in the appropriate creator methods.

[fingolfin]

@hulpke you wrote:

Would it be possible to temporarily (just for tests) make this an immediate method that will trigger an error for groups?

I don't understand what you mean here: IsGroup is a subfilter of IsMagmaWithInverses, so an immediate method should never trigger for anything in IsGroup to start with. To the contrary, it causes things to notice they are groups which wouldn't know otherwise. Perhaps you meant we should test if it ever triggers now, and for what it does so?

[hulpke]

@fingolfin
Sorry if my comment was unclear.

Perhaps you meant we should test if it ever triggers now, and for what it does so?

Yes, this is a method that should never trigger, but if it does something is wrong. Thus my request:

• Add such a method
• Run the test suite (all of tst and manual) once, see if it triggers
• If indeed it does not trigger remove the code again and in the git comment simply note that it never got triggered.
• If it does get triggered it is an indication that we were building a dodgy object.

[fingolfin]

OK. I've now added a commit with such an immediate method (so you can check if that's what you had in mind). Let's see what the Travis tests report (locally, I got no errors with testinstall, but let's wait for the whole test suite).

[codecov]

Codecov Report

Merging #2452 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
+ Coverage   74.01%   74.01%   +<.01%     
==========================================
  Files         484      484              
  Lines      245371   245371              
==========================================
+ Hits       181615   181622       +7     
+ Misses      63756    63749       -7

Impacted Files	Coverage Δ
lib/magma.gd	100% <ø> (ø)	⬆️
lib/addmagma.gi	27.05% <100%> (+1.17%)	⬆️
src/objset.c	84.82% <0%> (+0.22%)	⬆️
hpcgap/lib/hpc/stdtasks.g	64.7% <0%> (+0.76%)	⬆️

[fingolfin]

Actually, IsMagmaWithInverses is a category, and immediate methods are not run for those (they only can be used to imply things with a tester, i.e. properties and attributes).

Hence adding this error does not have the desired effect.

There is one example in the tests were removing the implication had an effect, though:

########> Diff in /home/travis/build/gap-system/gap/tst/testbugfix/2017-10-23-\
MagmaWith.tst:3
# Input is:
MagmaWithOne(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
# Expected output:
Group([ (1,2), (1,2,3) ])
# But found:
<monoid with 2 generators>

This could of course easily be "fixed" by teaching MagmaWithOne to set the category IsMagmaWithInverses if appropriate. Though I wonder if this is really needed / desirable -- it's hard for me to tell, as I find this part of the GAP architecture rather odd: I explicitly create something as a magma, an want to study it as a magma, but GAP somehow decided that since my magma is also a group, it should be treated as a group by setting a category. But for other magmas, which also are groups, GAP won't notice, and thus won't set that category. Same for semigroups and monoids.

Also, GAP distinguishes between ring and rings-with-one -- and if a ring has a one, it is not automatically "upgraded" to a ring-with-one. Why do we do that for magmas and semigroups?

I guess I am with @mohamed-barakat here: I wish that if I told GAP that I want to create e.g. a magma or a semigroup, it'd give me one. If it turns out to be a group, too, let me us AsGroup to convert it, but don't try to be clever and second guess me.... Alas, I guess that ship has sailed.

But that still leaves me wondering what a "correct" solution for the "problem" (???) at hand is...

[hulpke]

@fingolfin
Thanks for attempting this test (though it seems useless after all).

OTOH your example is weird, in particular becauseSubgroup(NC) is defined as synonym of SubmagmaWithInversesNC and thus is created as a Magma.

I think the only clean solution is to abolish the automatic upgrades to groups, i.e. declaring your example as compliant. Who creates a magma shall get a magma and has no reason to expect a group suddenly.
This way GAP becomes more consistent, might serve some users (such as Mohamed) better, at the small cost of losing some show-off magic that is not really helping.

In this case we need do make SubgroupNC its own operation, but that should not be hard. It also might need a few test/manual changes which explicitly proclaimed the upgrade policy.

[fingolfin]

I updated the tests to work (we'll see if Travis confirms).

At this point I am not quite sure what if anything remains to be done in this PR. Sure, we may want to overhaul Subgroup etc. on the long run -- but is there any issue with this PR? If so, what is it?

[hulpke]

@fingolfin My (hypothetical) concern is that code that used to produce SubGroups now produces submagmas and that leads to calculations moving on the generic track. It would be good to test not only the standard test suite, but also testextra and the test benchmarks (which are inherently longer calculations.

I can look at that once #2387 (which also still is waiting for an approval) has been merged.

[fingolfin]

@hulpke it wasn't clear to me at all that PR #2387 is now ready for review, I understood your last comment there as saying the opposite, hence I stopped looking at it. I am now busy with other stuff, but I'll try to review it when I find some time.