Turn hidden implications into actual implications

[fingolfin]

This implements parts of the proposal for fixing #1649, by turning many "hidden" implications into actual implications. I think this is beneficial for its own sake, although the long term plan is to try and see if we can get rid of hidden implication; see also PRs #2246 and #2247.

See also the new issue #2336 for the overarching goal of getting rid of hidden implications.

OLD TEXT follows.

With this applied, I can remove the code for setting / handling hidden implications and start GAP with -A and get no warnings. However, some tests do not pass if I do that. Beyond that, many packages need InstallTrueMethod calls before we can consider removing hidden implications.

There are three [UPDATE: four] things I'd like to do before merging this PR:

1. Merge PRs Fix implication for IsSubsetLocallyFiniteGroup; and teach Units(GF(q)) its size #2220 and Bug fix: IsRightAlgebraModule is a right module, not left (DO NOT MERGE) #2221, which are required for this PR (and right now they are contained in it)
2. Add a helper function to get the ranks of all filters; then use that to compare them before and after this PR, and also after removing hidden implications; deal with any rank changes.
3. Investigate and resolve the test failures when hidden implications are disabled (I hope point 2 4 will help with this).
4. [NEW] As suggested by @ThomasBreuer in issue Wrong warning: method installed for * matches more than one declaration #1649, it would be very useful to have another helper function which checks for all operations whether the relative rank of their methods changes w/o hidden implications.

[fingolfin]

Regarding point 2, I wrote a super simple script, and compared its output between this PR and master:

for n in NamesGVars() do
    if not IsBoundGlobal(n) then continue; fi;
    f:=ValueGlobal(n);
    if IsFilter(f) then
        Print(n, ": ", RankFilter(f), "\n");
    fi;
od;

The good news: they are identical!

The bad news: if I remove hidden implications, they start to differ radically. To get an overview, I run the following on the master branch and on this PR; the code mimics RankFilter with hidden imps off.

count:=0;;
for n in NamesGVars() do
    if not IsBoundGlobal(n) then continue; fi;
    f:=ValueGlobal(n);
    if IsFilter(f) then
        rank := RankFilter(f);
        rank_no_hidden := Sum(RANK_FILTERS{TRUES_FLAGS(WITH_IMPS_FLAGS(FLAGS_FILTER(f)))});
        if rank <> rank_no_hidden then
            Print(n, ": ", rank, " vs ", rank_no_hidden, "\n");
            count := count + 1;
        fi;
    fi;
od;
Print("Total differences: ", count, "\n");

The result is that on master, there are 1509 diffs (with -A), with this PR still 1410. The vast majority of the diffs come from testers, e.g.:

...
HasIsDivisionRing: 26 vs 6
HasIsDuplicateFree: 2 vs 1
HasIsDuplicateFreeList: 2 vs 1
HasIsDuplicateTable: 21 vs 1
HasIsElementaryAbelian: 36 vs 1
HasIsEmpty: 2 vs 1
HasIsEndoGeneralMapping: 8 vs 1
HasIsEndoMapping: 10 vs 3
HasIsEquivalenceRelation: 10 vs 1
HasIsFamilyPcgs: 22 vs 1
HasIsField: 27 vs 7
HasIsFieldHomomorphism: 8 vs 1
HasIsFinite: 3 vs 1
HasIsFiniteDimensional: 24 vs 1
HasIsFiniteOrdersPcgs: 20 vs 1
HasIsFiniteSemigroupGreensRelation: 23 vs 1
HasIsFinitelyGeneratedGroup: 36 vs 1
HasIsFinitelyGeneratedMonoid: 21 vs 1
...

But also:

IS_NSORT_LIST: 5 vs 2
IS_PLIST_REP: 21 vs 3
IS_POSS_LIST: 5 vs 2
IsAbelian: 13 vs 2
IsAbelianTom: 3 vs 2
IsAdditivelyCommutative: 12 vs 2
IsAlmostSimpleCharacterTable: 22 vs 2
...
IsDistributive: 17 vs 4
IsDuplicateFree: 3 vs 2
IsElementFinitePolycyclicGroupCollection: 39 vs 15
IsElementaryAbelian: 47 vs 2
IsEmpty: 3 vs 2
IsFinite: 4 vs 2
IsFiniteDimensional: 25 vs 2
IsFiniteSemigroupGreensRelation: 24 vs 2
...
IsNInfinity: 15 vs 2
IsNSortedList: 5 vs 2
IsNaN: 15 vs 2
IsNaturalGL: 49 vs 39
IsNaturalSL: 51 vs 41
IsNearRing: 26 vs 25

In the end, I guess most of this will be OK, but perhaps we need to check some things... E.g. perhaps some explicit implications are needed for tests? E.g. with master and this PR, I see this:

gap> RankFilter(IsDihedralGroup); RankFilter(IsDihedralGroup and IsGroup);
37
37
gap> RankFilter(HasIsDihedralGroup); RankFilter(HasIsDihedralGroup and IsGroup);
36
36

With this PR and hidden imps turned off, I get this:

gap> RankFilter(IsDihedralGroup); RankFilter(IsDihedralGroup and IsGroup);
37
37
gap> RankFilter(HasIsDihedralGroup); RankFilter(HasIsDihedralGroup and IsGroup);
1
36

One may now argue that HasIsDihedralGroup should imply IsGroup. If we agree on that, I could change this PR to do so.

[fingolfin]

Here is a simple program to deal with point 4, i.e., determine whether methods in some operations changed:

# scan global variables for operations
count := 0;
ops_with_diffs := 0;
meths_with_diffs := 0;
RankFilterNoHidden:=function(f)
    if IsFunction(f) then f:=FLAGS_FILTER(f); fi;
    return Sum(RANK_FILTERS{TRUES_FLAGS(WITH_IMPS_FLAGS(f))});
end;

# give all filters which have hidden implications from <filter>, but
# not actual impllications;
HiddenImpliedFilters:=function(filter)
    local flags, imps, hidden, diff;
    flags := FLAGS_FILTER(filter);
    imps := WITH_IMPS_FLAGS(flags);
    hidden := WITH_HIDDEN_IMPS_FLAGS(flags);
    diff := Difference(TRUES_FLAGS(hidden),TRUES_FLAGS(imps));
    return FILTERS{diff};
end;

for name in NamesGVars() do
    if not IsBoundGlobal(name) then continue; fi;
    v:=ValueGlobal(name);
    if not IsOperation(v) then continue; fi;
    count := count + 1;
    # FIXME: skip constructors for now
    if IS_CONSTRUCTOR(v) then continue; fi;
    # iterate over all methods
    for n in [0..7] do
        methods := METHODS_OPERATION(v, n);
        ranks_old := [];
        ranks_new := [];
        for i in [1..Length(methods)/(n+4)] do
            filters := methods{[(n+4)*(i-1)+2 .. (n+4)*(i-1)+1+n]};
            rank := methods[(n+4)*(i-1)+3+n];
            rank_bonus := rank - Sum(filters, RankFilter);
            #Assert(0, rank_bonus >= 0);
            Add(ranks_old, rank);
            Add(ranks_new, rank_bonus + Sum(filters, RankFilterNoHidden));
        od;
        Assert(0, IsSortedList(Reversed(ranks_old)));
        if not IsSortedList(Reversed(ranks_new)) then
            ops_with_diffs := ops_with_diffs + 1;
            # find positions with changed order
            bad := Filtered([1..Length(ranks_new)-1], i-> ranks_new[i] < ranks_new[i+1]);
            #Error("foo");
            Print("Method order change for ", name, ": ", bad, "\n");
            meths_with_diffs := meths_with_diffs + Length(bad);
            for i in bad do
                info := methods[(n+4)*(i-1)+4+n];
                func := methods[(n+4)*(i-1)+2+n];
                loc := LocationFunc(func);
                if IsEmpty(loc) then 
                    loc := "???";
                fi;

                info2 := methods[(n+4)*(i)+4+n];
                func2 := methods[(n+4)*(i)+2+n];
                loc2 := LocationFunc(func);
                if IsEmpty(loc2) then 
                    loc2 := "???";
                fi;

                Print("   '", info, "' at ", loc, "\n");
                Print("        old rank ", ranks_old[i], "; new rank ", ranks_new[i], "\n");
                Print("     versus\n");
                Print("   '", info2, "' at ", loc2, "\n");
                Print("        old rank ", ranks_old[i+1], "; new rank ", ranks_new[i+1], "\n");
                Print("\n");
            od;
        fi;
    od;
od;
Print("Total number of operations checked: ", count, "\n");
Print("  ", ops_with_diffs, " had methods whose relative rank changed\n");
Print("  about ", meths_with_diffs, " methods were reordered\n");

Summary of the results: In master I get this:

Total number of operations checked: 5923
  208 had methods whose relative rank changed
  about 516 methods were reordered

With this PR, I get:

Total number of operations checked: 5923
  110 had methods whose relative rank changed
  about 233 methods were reordered

Here is a typical bit of output:

...
Method order change for {}: [ 5 ]
   '{}: for a small list and a small dense list' at src/lists.c:ELMS_LIST_DEFAULT
        old rank 18; new rank 13
     versus
   '{}: for a plist matrix and a list' at src/lists.c:ELMS_LIST_DEFAULT
        old rank 17; new rank 17
...

The helper function HiddenImpliedFilters added by the above code snippet reveals why:

gap> HiddenImpliedFilters(IsList and IsDenseList and IsSmallList);
[ <Category "IsHomogeneousList">, <Category "IsCollection"> ]

OK, we definitely do not want to have these as actual implications. And in this particular example, the order change did not matter, as both methods end up calling the same function anyway (though that does not yet quite mean things are fine, because I only print neighbors which are ordered wrong, and not all methods which changed relative order).

Anyway, this means that in order to remove hidden implications, quite a lot more work is needed.

[fingolfin]

Note that many of the changes in relative order can be resolved by additional implications, the tricky part is finding them and getting the implications right. Example:

gap> HiddenImpliedFilters(IsPositiveIntegers);
[ <Category "CategoryCollections(IsNearAdditiveElementWithZero)">, <Category "CategoryCollections(IsNearAdditiveElementWithInverse)"> ]

So, yeah, it would make sense to have IsPositiveIntegers extend the category IsNearAdditiveElementWithZeroCollection. And of course I can just bluntly do that. But I wonder if this is the "right" fix... It seems so arbitrary...

[ThomasBreuer]

As far as I understand the intended setup, the proposed additions of explicit implications
(via InstallTrueMethod) are needed in order to achieve proper relative ranks for the methods.
The documentation will eventually state that such implications can be both logically sensible
and useful in order to simplify method installations.

However, these implications can also be dangerous in the situation where an attribute/property
gets declared several times.
For example, if two packages declare the same property (for different kinds of objects)
and if both of them install an implication to some filter which belongs to the package in question
then setting the property will set the filters for the two different contexts,
which may lead to strange objects.

Would it perhaps make sense to install the implication not via a separate call of InstallTrueMethod
but via an optional argument of DeclareProperty etc.,
and to signal an error if an existing property gets redeclared such that at least one of the declarations has required the implication to its given filter (and if the given filters are not compatible)?

[hulpke]

[This is concerning change 0365e99, Remove invalid implication for IsSubsetLocallyFiniteGroup, only]

I don't understand this. IsFFE are elements of a finite degree extension of GF(p). If I have finitely many of them they all lie in a finite degree extension. What Do I miss?

Also I suggest to be careful with simply removing implications without checking why they were added in the first place. There can be situations where removing an implication of finiteness or so can suddenly cause certain calculations go very slow.

[ThomasBreuer]

@hulpke: The removed implication installation is

InstallTrueMethod( IsSubsetLocallyFiniteGroup, IsFFECollection and IsMagma );

The point is that IsFFECollection and IsMagma allows for zero elements, and these cannot lie in a group.
One could replace the condition IsMagma by IsMagmaWithInverses, then the implication would be correct.

[fingolfin]

@hulpke First off, that change also has its own PR #2220. You write "Also I suggest to be careful with simply removing implications without checking why they were added in the first place", which seems to suggest that I am removing an implication without checking why it was added in the first place. But I did! I also did not "simply remove it", instead I submitted a PR, with the understanding that people will review it, and hopefully catch if I made a mistake, which of course can happen despite me taking great care in what changes I make. So, I heartily invite you to comment on PR #2220. I suggest we discuss this further on PR #2220, not here, as this commit is only in this PR because the changes in here revealed this incorrect implication and turned it into a serious problem (e.g. GF(2) suddenly was an in the filter IsGroup, despite containing a non-invertible element). BTW, this PR here is also marked as "work in progress" and "do not merge", which is why I did not explicitly point out this particular change, other than pointing to PR #2220.

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2222      +/-   ##
==========================================
+ Coverage   74.27%   74.27%   +<.01%     
==========================================
  Files         484      484              
  Lines      245343   245343              
==========================================
+ Hits       182218   182225       +7     
+ Misses      63125    63118       -7

Impacted Files	Coverage Δ
hpcgap/lib/basis.gd	100% <ø> (ø)	⬆️
lib/basis.gd	100% <ø> (ø)	⬆️
lib/coll.gd	93.52% <ø> (ø)	⬆️
lib/fldabnum.gd	100% <ø> (ø)	⬆️
lib/grppc.gi	71.69% <ø> (ø)	⬆️
lib/grppcatr.gi	80.26% <ø> (ø)	⬆️
lib/methwhy.g	50% <ø> (ø)	⬆️
lib/ghom.gd	93.33% <100%> (ø)	⬆️
lib/tuples.gi	90.66% <0%> (-2%)	⬇️
src/c_oper1.c	90.88% <0%> (+0.27%)	⬆️
... and 2 more

[fingolfin]

@ThomasBreuer You are of course right, one has to be careful with this implications. For this PR, I restricted myself to implications which are "obviously right", e.g. because the filter name already suggest it: IsPGroup implies IsGroup, and any package which declares a property IsPGroup for something which is not a group arguably is broken. OTOH, IsAbelian can sensibly be applied to things which are not groups, so I did not add an implication.

That said, mistakes happen, and if GAP can do something to give a helpful warning, then we should do that. In so far, I like you suggestion of adding an extra argument to DeclareProperty which sets the reverse implication and also ensures that another DeclareProperty for the same name will give an error. This would then also make it more convenient to declare such reverse implications.
(As a minor alternative to adding another argument, we could also add a whole new function, say DeclarePropertyWithReverseImplication, or DeclareExclusiveProperty or whatever).

This would then also allow us choose between the implication IsProperty => IsRequirement and HasIsProperty => IsRequirement by changing a single line inside DeclareProperty resp. DeclareExclusiveProperty.

I can implement this in this PR, but I'll wait a bit to see if others have thoughts on this idea.

[frankluebeck]

Ah, I have actually largely done what I proposed in #1649. But I didn't find time to write up a detailed explanation and to finish all tests I wanted to do. Therefore, it it not yet pushed.

If there is interest, I can try to turn my two pull requests into a presentable state within the next days.

[ThomasBreuer]

I had a closer look at the proposed changes, and suggest the following.

• Setting IsPrimeField should also set IsCommutative.
  As far as I see, this implication is currently not installed.
  (The declaration of the property IsPrimeField for objects in IsDivisionRing is correct.)

• The declarations of GroupByGenerators and GroupWithGenerators for one argument
  should require this argument to be in IsCollection, not just in IsListOrCollection.
  Contrary to the case of two arguments, the empty list is not admissible in this situation.

[fingolfin]

@ThomasBreuer thanks, took care of both your points

[fingolfin]

@frankluebeck sure, if you have such code, it certainly would be interesting to see it in a PR (or multiple)

[fingolfin]

The remaining test failure is caused by issue #2252.

[fingolfin]

Rebased this PR and adjusted for recent changes. If all tests pass, I think it could be merged now.

[fingolfin]

To give an example of why I think this PR is already now useful, consider this example, in the master branch:

gap> ShowImpliedFilters(IsSSortedList);
Implies:
   IsSortedList


May imply with:
+IsList
   IsListOrCollection
   IsDuplicateFree

and with this PR:

gap> ShowImpliedFilters(IsSSortedList);
Implies:
   IsList
   IsListOrCollection
   IsSortedList
   IsDuplicateFree

[ThomasBreuer]

In principle, I am in favor of these changes.
However, I do not see whether they could be improved
further by adopting parts of # 2247 (by @frankluebeck).
Since Frank is also asked for a review, I leave the decision to him.

[fingolfin]

I am sure further improvements are possible; perhaps some already are in PR #2247; most likely there are more not covered in either PR. But I don't see why the possibility of even further improvements should stop us from already adding some improvements right away?

[fingolfin]

@frankluebeck @ThomasBreuer I've now updated this, taking PR #2247 into account.

[fingolfin]

@frankluebeck @ThomasBreuer Ping? Still needs your approval...

[frankluebeck]

This looks fine. The current master branch behaves in many respects as if the explicit implications which are added here are already there (because of "hidden implications"). Making these implications explicit is an important step towards getting rid of the hidden implications.

I suggest to add the following three small changes:

diff --git a/lib/coll.gd b/lib/coll.gd
index e947887..51ff1ae 100644
--- a/lib/coll.gd
+++ b/lib/coll.gd
@@ -1395,7 +1395,7 @@ DeclareProperty( "IsNonTrivial", IsCollection );
 ##  </ManSection>
 ##  <#/GAPDoc>
 ##
-DeclareProperty( "IsFinite", IsCollection );
+DeclareProperty( "IsFinite", IsListOrCollection );
 
 InstallSubsetMaintenance( IsFinite,
     IsCollection and IsFinite, IsCollection );
diff --git a/lib/grp.gd b/lib/grp.gd
index ca7c815..ab4ea6f 100644
--- a/lib/grp.gd
+++ b/lib/grp.gd
@@ -119,6 +119,7 @@ InstallTrueMethod( IsFiniteOrderElementCollection, IsGroup and IsFinite );
 ##  <#/GAPDoc>
 ##
 DeclareSynonymAttr( "GeneratorsOfGroup", GeneratorsOfMagmaWithInverses );
+InstallTrueMethod(IsGroup, HasGeneratorsOfGroup);
 
 
 #############################################################################
diff --git a/lib/grppc.gi b/lib/grppc.gi
index e7e9e17..ad1964c 100644
--- a/lib/grppc.gi
+++ b/lib/grppc.gi
@@ -224,7 +224,7 @@ end);
 #M  Pcgs( <G> )
 ##
 InstallMethod( Pcgs, "fail if not solvable", true,
-        [ HasIsSolvableGroup ], 
+        [ IsGroup and HasIsSolvableGroup ], 
        SUM_FLAGS, # for groups for which we know that they are not solvable
                   # this is the best we can do.
     function( G )

Then one can use the -N switch from pull request "Introduce -N switch to not use hidden implications
" (#2246) and run gap -r -A -N tst/testinstall.g successfully (up to very few harmless differences).

Further explicit implications will be needed which will become clear by comparing filter ranks when gap is called with and without -N switch. But that should not stop us from commiting those provided here now.

[fingolfin]

@frankluebeck I added your first and last suggestion. I am a bit wary of the middle one, and would prefer if this was added in a future PR (if at all), instead of delaying this PR further, which has been cooking since Februar 28. (Specifically, this PR still has no explicit approval as described on https://help.github.com/articles/approving-a-pull-request-with-required-reviews/, so it cannot be merged).

The reason I am wary about the middle change is this: GeneratorsOfGroup is a synonym for GeneratorsOfMagmaWithInverses. But a magma with inverses need not be associative. With the implication you suggest, a non-associative magma with inverses would be automatically marked as a group, which is a no-go.

There are various ways to address this, but again, I'd prefer to do this in a separate fresh PR, instead of revising this one again and again; I think this PR already is useful, even if it does not yet resolve all warnings reported by your -N patch.

[frankluebeck]

see discussion for details

[frankluebeck]

@fingolfin: I understand the complaint about the change concerning GeneratorsOfGroup. But this probably means that the existing synonym setting of GeneratorsOfGroup and GeneratorsOfMagmaWithInverses is a bug?

[fingolfin]

@frankluebeck That may very well be! We recently discovered a bunch of issues of a very similar kind. Definitely should be investigated.

[fingolfin]

I'll submit an issue for it, so we don't forget