Reduce impact of immediate methods to calculations with groups #2387

hulpke · 2018-04-22T23:56:58Z

This is an attempt to resolve #2386. It changes group generation that many immediate methods are not triggered any longer, and removes immediate methods that are taken care off by the creation process or that have small payoff.

See the commit text for detailled description.

Note that it will require #2429 which was requested to be taken out (and might not test cleanly without)

fingolfin · 2018-04-23T06:35:58Z

lib/coll.gi

+#        TryNextMethod();
+#    fi;
+#    return infinity;
+#    end );


I agree this should not be an immediate method, but perhaps it should be retained as a regular method? Then with the IsAttributeStoringRep removed.

If we do so, we need to rank this high, so that it overrides any other.

(Also removing this immediate method broke specific tests for this method will need to be adjusted.)

fingolfin · 2018-04-23T06:39:56Z

lib/grp.gi

+#    else
+#      TryNextMethod();
+#    fi;
+#    end );


Fine by me.

Ooops, the next method after this one is incorrect: it does

if HasGeneratorsOfGroup( G ) and Length( GeneratorsOfGroup(G) ) = 1 then SetMinimalGeneratingSet(G,GeneratorsOfGroup(G));

which is incorrect if GeneratorsOfGroup(G) consists only of an identity element. This is of course currently hidden by the immediate method. Mental note to self: try to find a tests case triggering this (after the immediate is gone), and fix it. (But anybody else feel free to fix this first, I won't work on it today for sure ;-)

fingolfin · 2018-04-23T06:41:25Z

lib/grp.gi

+        typ:=typ and HasIsTrivial and HasIsNonTrivial;
+        triv:=true;
+        # basic consequence
+        typ:=typ and HasIsPerfectGroup and IsPerfectGroup;


I'd rather have a InstallTrueMethod( IsPerfectGroup, IsGroup and IsTrivial );, this will cover the above (with zero overhead).

Yes. That would be much nicer. Will change.

fingolfin · 2018-04-23T06:42:43Z

lib/grp.gi

+  else
+    ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens),
+                            IsEmpty,false,IsTrivial,triv,
+                            IsNonTrivial,not triv);


Hmm, why set IsTrivial resp. IsNonTrivial this way, instead of just anding them into typ like you do with e.g. IsCyclic? Does this have some functional difference?

If not, I'd just set this all in typ, then you can get rid of triv and simplify the code here and elsewhere accordingly.

I'd love to do. But how do I set a flag to `false' as part of a type?

IsTrivial and IsNonTrivial are twins that are set the opposite way and currently immediate methods are used to set either to false if the other is set.

If you just specify HasIsCyclic without IsCyclic, then this is equivalent to setting it to false.

In somewhat related news, I plan to add an InstallFalseMethod helper soon (see #235), at least for "elementary" properties (i.e. not involving and-ing together multiple properties).

Thanks! Will do so.

fingolfin · 2018-04-23T06:45:34Z

lib/grp.gi

-    G:=rec();
-    ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens));
+  fi;
+  typ:=NewType(fam,typ);


Just to make it explicit: the changes in this PR get rid of the cached values fam!.defaultFinitelyGeneratedGroupType and fam!.defaultGroupType. I assume that you verified that there is a net gain (probably because this initial type (almost?) always ended up being replaced by immediate methods anyway.

When I left the caching in I ran into problems -- a non-nilpotent group claimed to be nilpotent. The best I could come up (#2394 ) is that the cached type in fact gets changed later and thus cannot be cached. If I'm doing something wrong here I'd love to know (and then would add type caching again).

I am not sure how useful those caches would be, unless done very carefully: in the old code, there were already two caches, one for finitely generated groups, and one for those where we don't know whether they are f.g. or not. In order to preset IsTrivial and other flags, you'd have to either create more caches (i.e. have one cached type for trivial groups; one for non-trivial cyclic; one for non-cyclic f.g.; one for the generic case). Of course that number grows as you add special cases. Luckily, in this case at least the number of types does not seem to grow exponentially, as most of the checks differentiate based on the generating set along.

At the start of writing the previous comment, I wasn't sure, because I thought you'd need to add 2^n caches, for n 3 or 4, but now it seems only four may be needed, which is at least not totally crazy.

Another cause for the problems you have seen may be that you add e.g. HasIsTrivial to the flag lists of the type. I think that means that any object created with that type is non-trivial, no matter what; once HasIsTrivial is set, one cannot change the value of IsTrivial anymore (at least not without low-level bit fiddling); this is a feature, not a bug.

fingolfin · 2018-04-23T06:51:12Z

lib/grp.gi

+    typ:=typ and IsFinitelyGeneratedGroup;
+    if Length(gens)<=1 then
+      typ:=typ and HasIsCyclic and IsCyclic 
+          and HasIsCommutative and IsCommutative;


Note that this is equivalent to typ := typ and IsCyclic, as IsCyclic implies IsCommutative, and each implies its tester, and handling these implications comes at zero cost here.

Ah -- I wasn't sure. Will fix.

Every property implies it tester; for the rest, you can check this e.g. as follows :

gap> ShowImpliedFilters(IsCyclic); May imply with: +IsMagmaWithInverses +IsAssociative IsCommutative IsNilpotentGroup IsSupersolvableGroup IsSolvableGroup IsNilpotentByFinite +IsOrdinaryTable IsCommutative IsNilpotentCharacterTable IsSolvableCharacterTable IsSupersolvableCharacterTable

Or, more technically:

gap> IsEquivFilters:={f1,f2}->IS_EQUAL_FLAGS(WITH_IMPS_FLAGS(FLAGS_FILTER(f1)), WITH_IMPS_FLAGS(FLAGS_FILTER(f2))); function( f1, f2 ) ... end gap> IsEquivFilters(IsGroup and IsCyclic, HasIsCyclic and IsCyclic and HasIsCommutative and IsCommutative and IsGroup); true

fingolfin · 2018-04-23T06:53:39Z

lib/grp.gi

+        and HasGeneratorsOfMagmaWithInverses and HasOne
+        and IsFinitelyGeneratedGroup and HasIsTrivial and HasIsNonTrivial
+        and HasIsFinite and HasIsCyclic
+        and HasIsPerfectGroup and HasIsEmpty;


Can't you just replace the above with typ:= IsGroup and IsAttributeStoringRep and HasGeneratorsOfMagmaWithInverses and HasOne and IsTrivial ? The IsTrivial implies (via zero cost implications) all the remaining flags.

fingolfin · 2018-04-23T06:54:49Z

lib/grppc.gi

@@ -272,7 +272,8 @@ InstallMethod( Pcgs,
 ##
 #M  GeneralizedPcgs( <G> )
 ##
-InstallImmediateMethod( GeneralizedPcgs, IsGroup and HasPcgs, 0, Pcgs );
+# is now set when creating
+#InstallImmediateMethod( GeneralizedPcgs, IsGroup and HasPcgs, 0, Pcgs );


Well, it is set when creating via GroupWithGenerators, but that's not the only way to create a group object! However, I agree this does not need to be an immediate method; esp. as nothing tests HasGroupWithGenerators. So let's just turn it into a regular method.

fingolfin · 2018-04-23T06:57:11Z

lib/magma.gi

-      TryNextMethod();
-    fi;
-    end );
+# this is now set when creating groups


This should say "when creating magmas". But even then it's not quite true: If a magma is created in a non-standard way, it may not be set. So, like with most of the other immediate methods you remove/commented out, I'd prefer to instead turn it into a regular method.

fingolfin · 2018-04-23T07:00:06Z

lib/magma.gi

+    # typ:=typ and IsFinitelyGeneratedGroup; don't know whether group
+    if Length(gens)<=1 then
+      typ:=typ and HasIsCyclic and IsCyclic 
+          and HasIsCommutative and IsCommutative;


This deduction is not correct for non-associative magmas.

codecov · 2018-04-23T17:22:06Z

Codecov Report

Merging #2387 into master will decrease coverage by <.01%.
The diff coverage is 94.34%.

@@            Coverage Diff             @@
##           master    #2387      +/-   ##
==========================================
- Coverage   73.98%   73.98%   -0.01%     
==========================================
  Files         484      484              
  Lines      245405   245434      +29     
==========================================
+ Hits       181571   181573       +2     
- Misses      63834    63861      +27

Impacted Files	Coverage Δ
lib/coll.gd	`93.52% <ø> (ø)`	⬆️
lib/ctbl.gd	`70.83% <ø> (ø)`	⬆️
lib/object.gd	`50% <ø> (ø)`	⬆️
lib/grpmat.gi	`68.27% <100%> (+0.36%)`	⬆️
lib/csetgrp.gi	`58.78% <100%> (-0.16%)`	⬇️
lib/grpfp.gi	`65.1% <100%> (+0.03%)`	⬆️
lib/grppc.gi	`72.18% <100%> (+0.49%)`	⬆️
lib/grppcext.gi	`50.47% <100%> (+0.1%)`	⬆️
lib/modulrow.gi	`91.35% <100%> (+0.19%)`	⬆️
lib/grppcaut.gi	`44.32% <50%> (+0.04%)`	⬆️
... and 31 more

fingolfin · 2018-04-23T20:12:12Z

lib/grp.gi

+      id:=One(gens[1]);
+      if ForAny(gens,x->x<>id) then
+        typ:=typ and HasIsTrivial and HasIsNonTrivial;
+        triv:=false;


Hmm, why does this still use triv instead of setting typ:=typ and HasIsTrivial and IsNonTrivial; similar below

fingolfin · 2018-04-23T20:16:32Z

lib/grp.gi

+      else
+        typ:=typ and HasIsTrivial and HasIsNonTrivial;
+        triv:=true;
+        typename:="defaultFinitelyGeneratedTrivialGroupType";


The FG part of this name seems redundant: trivial already implies iy, no? Same for the cyclic types.

fingolfin · 2018-04-23T20:19:11Z

lib/grp.gi

-      typ:=fam!.defaultGroupWithOneType;
+    elif Length(gens)<=1 then
+      typ:=typ and IsCyclic;
+      typename:="defaultFinitelyGeneratedCyclicGroupWithOneType";


This type computation is repeated several times across multiple GroupWithGenerators methods - can't it perhaps be factored out into a common helper function?

The best I was able to come up with would have been preprocessor macros, but for a function there are too many small differences that would require input parameter and extra processing. (This is an observation of my incompetence, not a statement that it cannot be done.)

fingolfin · 2018-04-23T20:19:48Z

lib/grp.gi

+#    IsSolvableGroup and HasIsTrivial,
+#    0,
+#    IsTrivial );
+InstallTrueMethod( IsPerfectGroup, IsGroup and IsTrivial );


InstallTrueMethod calls belong into .gd files

fingolfin · 2018-04-23T20:23:58Z

lib/coll.gi

+#  replace immediate method by ordinary, as immediate gets called for every
+#  group that knows it is finite but does not know its size -- e.g.
+#  permutation, pc.  The benefit of this is minimal beyond showing off a
+#  feature.


This comment (as well as several other that follow) only make full sense when read in the context of this commit resp. PR; but if somebody reads this in a year, they might have a hard time understanding what is meant here. "What is being replaced here???"

Not having this commit would IMHO be better; or start it with something like "the following method used to be immediate. However..." but I wonder whether this would be a useful comment. I think this would better fit into the commit message.

I'll change the text. IF it it hidden in a commit message, however I think it is less likely to be seen and also loses the reference to the place in the source code.

fingolfin · 2018-04-23T20:24:21Z

lib/coll.gi

+#  group that knows it is finite but does not know its size -- e.g.
+#  permutation, pc.  The benefit of this is minimal beyond showing off a
+#  feature.
+InstallMethod( Size,true, [IsCollection and HasIsFinite], 0,


You can omit true and 0 here

OK, but is it a problem if they're in? (Some methods actually might need a higher ranking).

fingolfin · 2018-04-23T20:24:38Z

lib/grp.gi

@@ -30,7 +30,9 @@ InstallImmediateMethod( IsFinitelyGeneratedGroup,
 ##
 #M  IsCyclic( <G> ) . . . . . . . . . . . . . . . . test if a group is cyclic
 ##
-InstallImmediateMethod( IsCyclic, IsGroup and HasGeneratorsOfGroup, 0,
+# this is now set when creating the groups, make ordinary instead of


fingolfin · 2018-04-23T20:26:12Z

lib/grp.gi

+        fi;
+      else
+        typ:=typ and IsTrivial and HasIsNonTrivial;
+        typename:="defaultFinitelyGeneratedTrivialGroupType";


The FG part of this name seems redundant: trivial already implies iy, no? Same for the cyclic types.

yes, changed.

fingolfin · 2018-04-23T20:31:45Z

lib/grppcfp.gi

@@ -69,6 +69,7 @@ InstallGlobalFunction( IsomorphismFpGroupByPcgs, function( pcgs, str )
        od;
    od;
    H := F / rels;
+    SetIsFinite(H,true);


I find it rather awkward to add all this SetIsFinite calls before each SetSize; and it is easy to forget, too. I'd rather add this into a custome setter for Size. I can submit a PR doing that tomorrow. Such a setter could also set IsTrivial, IsEmpty, IsNontrivial in one go.

Also, I assume this is only beneficial because setting IsFinite does not trigger any/fewer immediate methods than setting Size, correct?

Yes to both. The setter also is on my to-do list.

Also, the IsFinite might be overkill -- I was trying to get rid of calls to immediate methods.

fingolfin · 2018-04-24T13:45:12Z

lib/grp.gi

+#InstallImmediateMethod( IsPerfectGroup,
+#    IsSolvableGroup and HasIsTrivial,
+#    0,
+#    IsTrivial );


Let's just remove this completely -- see also my suggestion above for the other "missing" direction.

fingolfin · 2018-04-24T14:08:57Z

lib/grp.gi

+    typ:=typ and IsFinitelyGeneratedGroup;
+    typename:="defaultFinitelyGeneratedGroupWithOneType";
+
+    if Length(gens)>0 and CanEasilyCompareElements(gens) then


Use >=0 to deal with empty collections? Or even better, as this also works if CanEasilyCompareElements(gens) is false, start like this:

if Length(gens) = 0 then ... elif CanEasilyCompareElements(gens) then ... elif Length(gens) = 1 then ... fi;

But there is a specialized method below for the case of IsEmpty for the collection gens. Wouldn't it override it?

That method is only for empty lists, but not for empty collections-that-aren't-lists. Though these are rather rare and special beast.

But as I said before, it would also be nice if all this logic would not have to be repeated in almost identical form several times, but rather was put into one (or at least very few) helper functions which can be called from multiple places.

I hear what you say and there is still stuff to clean up. If the type caching goes away, it should be easier to have the logic in one function, and I consider this a goal before merging.

The deductions logic is now in a separate function.

fingolfin · 2018-04-24T14:10:14Z

lib/grp.gi

+        typ:=typ and HasIsTrivial and IsNonTrivial;
+        if Length(gens)<=1 then
+          typ:=typ and IsCyclic;
+          typename:="defaultNontrivialCyclicGroupWithOneType";


BTW, NEW_TYPE also performs caching of types for a given family. You can inspect NEW_TYPE_CACHE_MISS and NEW_TYPE_CACHE_HIT before/after creating a group to see it was effective. So.... that might in fact render the cache here moot. (While the explicit cache here may be a tiny bit faster, it also adds a lot of complexity...)

I'll put this in a separate remark as a push will make it go outdated in code

(after all it did not, but should not be only commit remark)

fingolfin · 2018-04-24T14:10:51Z

tst/testinstall/coll.tst

-gap> SetIsFinite(c2, false);
-gap> HasSize(c2);
-true
-gap> Size(c2);


This test foremost was meant to test that Size method. Since it is still there, just not immediate, we could keep the test, and just remove the two checks for HasSize.

hulpke · 2018-04-24T16:15:58Z

@fingolfin remarked

BTW, NEW_TYPE also performs caching of types for a given family. You can inspect NEW_TYPE_CACHE_MISS and NEW_TYPE_CACHE_HIT before/after creating a group to see it was effective. So.... that might in fact render the cache here moot. (While the explicit cache here may be a tiny bit faster, it also adds a lot of complexity...)

Yes, the type caching here is beyond ugly, and the only reason would be if it has a notable performance impact.

On a single pair of tests I measure (re-starting GAP) 237 sec. with caching versus 211 seconds without caching. Is this good enough to say we simply leave the caching out?

In any case, I have rearranged the commits so that the re-introduction of caching (with shorter names) now is all located in a single commit. It thus could be removed easily.

fingolfin · 2018-04-24T19:36:55Z

lib/grp.gi

@@ -30,7 +30,9 @@ InstallImmediateMethod( IsFinitelyGeneratedGroup,
 ##
 #M  IsCyclic( <G> ) . . . . . . . . . . . . . . . . test if a group is cyclic
 ##
-InstallImmediateMethod( IsCyclic, IsGroup and HasGeneratorsOfGroup, 0,
+#  This used to be an immediate method. It was replaced by an ordinary
+#  method sine the flag is typically set when creating the group.


sine -> since

fingolfin · 2018-04-26T19:40:22Z

lib/grppcaut.gi

@@ -971,6 +975,7 @@ local f,act,xset,stb,hom,n,m,l,i;
    Add(stb,m);
    stb:=List(stb,x->x^act); # base change
    stb:=Group(stb);
+    SetIsFinite(stb,true);


Are all these SetIsFinite calls still necessary?

No. SetSize (or SetIsGroupOfAutomorphismsFiniteGroup) should take care of it.

fingolfin · 2018-04-27T23:08:16Z

lib/coll.gi

+    SetIsEmpty(obj,sz=0);
+    SetIsTrivial(obj,sz=1);
+    SetIsNonTrivial(obj,sz<>1);
+    SetIsFinite(obj,sz<>infinity);


Doesn't this still change the type four times (less for sz=0 and sz=1 because of implications, but stilll). Plus a fifth time for the size. So, how about something like this:

function(obj, size) local filt; if HasSize(obj) and Size(obj) <> size then ErrorNoReturn("size of <obj>" already set to ", Size(obj), ", cannot change it to ", size); fi; if size = 0 then filt := IsEmpty; elif size = 1 then filt := IsTrivial; elif size = infinity then filt := IsNonTrivial and HasIsFinite; else filt := IsNonTrivial and IsFinite; fi; filt := filt and HasSize; obj!.Size := size; SetFilterObj(obj, filt); end;

This assumes we already have or add (negative) implications between IsEmpty, IsTrivial and IsNonTrivial (the three are after all mutually exclusive).

(We may also wish to add IsNonEmpty and IsInfinite, but better not in this PR)

Thank you -- you've just answered a question I had, but had not yet asked -- indeed the four sets are bad, but I wanted first to get the errors out. Will change as you suggest.

This is done. However (the manual specifies that an SetSize if size is given has no effect) I have wrapped the error into a check that requires assertion level to be >=3.

hulpke · 2018-05-01T15:20:34Z

@fingolfin
I have addressed all remarks. The code will throw errors on test examples at assertion level 3, this goes away once #2429 is merged.

fingolfin

Only some quick remarks, I have not yet looked at everything, need to prepare class material etc. now instead.

However, I am rather unhappy with the commit structure. At the very least, the commit messages need work. They reference work which is not visible anymore, which is highly confusing. They also contain typos (e.g. Empty instead of IsEmpty), and incomplete sentences, reducing their usefulness.

However, it also happens that one commit introduces a typo and then a later one fixes it (instead of just fixing the first commit); or that a commit contains changed not indicated by its summary line (which then turns out to not actually summarize what the commit is about).

I know it's tiresome to work so long on such a big patch, but it's also very tiresome if one has to debug these changes in a year down the road, and has to painstakingly reverse engineer what the changes were about, etc., so doing this right now will save us work down the road.

In this case, it might actually be best to first squash all changes together into one commit, or perhaps two (using git reset to uncommit all changes, followed by using git add -p judiciously to create fresh commits): one which turns immediates into implications; and one with the rest. I haven't had a chance to read through everything to say for sure, I'll try to do that during this week.

fingolfin · 2018-05-01T19:19:23Z

lib/coll.gi

+  if HasSize(obj) and Size(obj)<>sz then
+    if AssertionLevel()>2 then
+      # Make this an ordinary error to enter break loop so that one can
+      # investigate call order for debugging


I don't understand this comment. What is a "non-ordinary error"? Perhaps you are referring to Assert? But that enters a break loop for me just fine, doesn't it?

You had suggested ErrorNoReturn in an earlier remark on this function. For debugging (and understanding how code worked in the past despite this issue) it seemed better to not forbid return;. (Yes, it could mislead a user, but this is really an error that should never come up for a user. (I'm happy to replace by Assert, if that is preferred here.

fingolfin · 2018-05-01T19:20:42Z

lib/coll.gi

@@ -3052,6 +3053,31 @@ end);
 InstallMethod( CanComputeIsSubset,"default: no, unless identical",
  [IsObject,IsObject],IsIdenticalObj);

+# avoid immediate methods triggering multiple type changes once the Size of
+# an object is known.


This comment starts mid-thought. Perhaps: "Custom SetSize method which sets various properties implied by the given size. This avoids ..." ?

I did not see this as mid-thought, but I'm happy to change it.

fingolfin · 2018-05-01T19:21:57Z

lib/csetgrp.gi

    RightCosetCanonicalRepresentativeDeterminator);
+  SetSize(d,Size(U)); # as own setter


What does "as own setter" mean?

Also, doesn't ObjectifyWithAttributes call custom setters anyway? It contains specific code to detect them. But perhaps you want to avoid that? In that case, I guess a comment explaining that (and why) would be indeed appropriate, but this comment isn't doing that.

We cannot set the size in the previous ObjectifyWithAttributes as there is a custom setter method (the one just added here). In such a case ObjectifyWith Attributes just does Objectify` and calls all setters separately which is what we want to avoid here.

lib/grp.gd

@@ -784,6 +784,8 @@ InstallFactorMaintenance( IsSolvableGroup,
 InstallTrueMethod( IsSolvableGroup, IsMonomialGroup );
 InstallTrueMethod( IsSolvableGroup, IsSupersolvableGroup );

+InstallTrueMethod( HasIsPerfectGroup, IsGroup and IsSolvableGroup and IsNonTrivial );


fingolfin · 2018-05-01T19:26:13Z

lib/grp.gi

@@ -31,7 +31,7 @@ InstallImmediateMethod( IsFinitelyGeneratedGroup,
 #M  IsCyclic( <G> ) . . . . . . . . . . . . . . . . test if a group is cyclic
 ##
 #  This used to be an immediate method. It was replaced by an ordinary
-#  method sine the flag is typically set when creating the group.
+#  method since the flag is typically set when creating the group.


This fixes a typo from the first commit. It should be instead melded into the first commit...

hulpke · 2018-05-01T21:00:26Z

@fingolfin
I understand that all of us have other work to do. (I will refrain on offering unsolicited opinion about the enforced review process.)

Yes, my plan is to merge everything into a single commit (and then revise the comments) before merging. (Some of this has been done now.)
The reason for doing so (instead of multiple single-topic commits) is that subsets of the commits do not really make sense once one starts dealing with the immediate methods in one context.

So far I have done partial merges but left some recent commits separately so it was clear what has changed.

fingolfin · 2018-05-04T22:22:16Z

@hulpke I misunderstood one of your earlier remarks as saying "this PR is now ready to be merged". I see now that this wasn't what you said at all. My apologies for the misunderstanding.

This commit changes routines that create groups, certain Submagmas aand vector spaces so that flags that set basic properties (being trivial, being empty, being cyclic) are set at the same time as the object was created. This functionality used to be provided through immediate methods, however this meant that the type of a just-created object was changed immediately multiple times (with some changes precipitating other changes), causing a notable performance hit. Notably, the following changes have been done: 1) In a number of methods used to create groups (or submagmas, when called from `Subgroup` or cosets) from generating sets, a number of cheaply deduced properties (such as being cyclic) are set while creating the object. Thus the immediate methods do not apply for these objects any longer. 2) Type caching in these methods is not worth the effort and has been removed. 3) A new setter method for `Size` deals similarly with deductions done before by immediate methods for a known size. Caveat: The manual specifies (ill-advised one might say) that setting a different size if a size already is given will be ignored. This is tested in manual examples and test files. The new setter method therefore cannot be used to check for this property as an error -- it will do so only if assertion level is set >=3. 4a) Immediate methods that have been made obsoleteby the changes in 1) and 3) have been changed to ordinary methods. 4b) A number of immediate methods (e.g. a trivial group is solvable) have been replaced by `TrueMethods` that have the same effect but are chaper to run. 4c) Also a number of immediate methods that need to run often, but apply only rarely (i.e. setting `IsFinite` to false if a size is set to `infinity`) have been removed. 5) These changes have minor impact on test files and manual tests: A few objects will have slightly different knowledge about their properties and thus print differently; respectivelty test code exists that checks explicitly for such properties having been set by immediate methods. These tests and examples have been changed. A few, very instable tests (e.g. checking explicitly for the values of random elements) have been commented out. 6) Some tests explicitly relied on immediate methods enabling derived series calculations in special cases of finitely presented groups. These tests never would have worked if immedite methods were turned off, the respective finitely presented groups method has been changed accordingly to test explicitly for the particular situation, thus enabling the same examples to work.

fingolfin

This looks pretty good now. I have some minor comments. Several of them (e.g. the wish to change IsTrivial and HasIsNonTrival to just IsTrivial`) can be addressed in a separate PR (and also by somebody else, e.g. me), I just mentioned them because I tried to document everything that occurred to me.

Anyway, I'd like to run a few more tests with this tonight, and (barring more remarks from @hulpke) plan to merge it then or tomorrow.

fingolfin · 2018-05-28T12:29:05Z

lib/csetgrp.gi

    RightCosetCanonicalRepresentativeDeterminator);
+  # We cannot set the size in the previous ObjectifyWithAttributes as there is
+  # a custom setter method (the one added in this commit). In such a case
+  # ObjectifyWith Attributes just does `Objectify` and calls all setters


"ObjectifyWith Attributes" -> "ObjectifyWithAttributes" (one word)

fingolfin · 2018-05-28T12:45:03Z

lib/grp.gi

-    IsSolvableGroup and HasIsTrivial,
-    0,
-    IsTrivial );
-


Why remove this, instead of turning this into a plain method? Wouldn't that still be useful?

(I don't think it's necessary to add a "this used to be an immediate method ..." comment)

fingolfin · 2018-05-28T12:48:13Z

lib/grp.gi

+          filt:=filt and IsTrivial and HasIsNonTrivial;
+        fi;
+      fi;
+    elif isgroup and Length(gens)<=1 then # cyclic not for magmas


You could replace this elsif by fi; if and then get rid of the identical check a few lines above.

fingolfin · 2018-05-28T12:53:44Z

lib/grpfp.gi

+    if Length(GeneratorsOfGroup(G))=1 then
+      SetIsAbelian(G,true);
+      return TrivialSubgroup(G);
+    fi;


So this seems to be a separate improvement for DerivedSubgroup on fp subgroups of rank 1... But why is it needed? Because we removed the immediate method for "size of generating set <= 1 implies abelian" ?
This raises two questions:

Where is the place we create a group w/o taking care of that? can we do something about it (i.e. fix the issue at its root, not the symptom)

Do we really need to remove (or replace by normal ones) those immediate methods? After all, if e.g. IsFinite is set, then no immediate method for IsFinite is run. So leaving those immediate methods in should not cause a speed penalty anymore -- or does it? I should run some tests....

fingolfin · 2018-05-28T12:56:40Z

lib/grppc.gi

+                          One, id,
+                          FamilyPcgs,pcgs,HomePcgs,pcgs,GeneralizedPcgs,pcgs);
+
+  #SetGroupOfPcgs (pcgs, G);


Why is this commented out? It used to be there. Is it not needed anymore? (Why?) If so, I'd just remove that line.

fingolfin · 2018-05-28T13:01:24Z

lib/grp.gi

+            filt:=filt and IsCyclic;
+          fi;
+        else
+          filt:=filt and IsTrivial and HasIsNonTrivial;


Since we now have implications IsTrivial => HasIsNonTrivial and IsNonTrivial => HasIsTrivial I'd kind of prefer if this line (and similar ones throughout this PR) were changed to e.g. filt:=filt and IsTrivial -- I find that much easier to grok; indeed, several times when reading through this PR did I misread a HasIsTrivial / IsTrivial / HasIsNonTrivial / IsNonTrivial

fingolfin · 2018-05-28T13:01:56Z

tst/testbugfix/00005.tst

@@ -15,4 +15,4 @@ gap> G:=GroupByGenerators( [ [ [ 0, -1, 0, 0 ], [ 1, 0, 0, 0 ],
 Group([ [ [ 0, -1, 0, 0 ], [ 1, 0, 0, 0 ], [ 0, 0, 0, -1 ], [ 0, 0, 1, 0 ] ] 
 ])
 gap> Centralizer(N,G);
-<matrix group of size infinity with 6 generators>
+<matrix group with 6 generators>


Interesting. Do you by chance know why we don't know that this centralize is infinite anymore?

hulpke force-pushed the ah/immediate branch from e3261f8 to 0ec2f7f Compare April 22, 2018 23:57

hulpke added request for comments do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: performance bugs or enhancements related to performance (improvements or regressions) labels Apr 22, 2018

hulpke force-pushed the ah/immediate branch from 0ec2f7f to 5135636 Compare April 22, 2018 23:59

fingolfin requested changes Apr 23, 2018

View reviewed changes

hulpke mentioned this pull request Apr 23, 2018

Caching Types #2394

Closed

hulpke force-pushed the ah/immediate branch from 5135636 to 7aec02d Compare April 23, 2018 17:21

hulpke force-pushed the ah/immediate branch from 093d077 to b746779 Compare April 23, 2018 20:20

fingolfin reviewed Apr 23, 2018

View reviewed changes

fingolfin reviewed Apr 24, 2018

View reviewed changes

hulpke force-pushed the ah/immediate branch 3 times, most recently from 959e9b6 to 718a107 Compare April 24, 2018 16:50

fingolfin reviewed Apr 24, 2018

View reviewed changes

hulpke force-pushed the ah/immediate branch 4 times, most recently from 9af1a23 to ed5c90a Compare April 24, 2018 22:08

hulpke mentioned this pull request Apr 25, 2018

Magma/Group confusion #2400

Closed

hulpke force-pushed the ah/immediate branch 2 times, most recently from 29132cf to 23258ff Compare April 26, 2018 16:06

fingolfin reviewed Apr 26, 2018

View reviewed changes

hulpke force-pushed the ah/immediate branch from 67c974a to f914dd1 Compare April 26, 2018 20:25

fingolfin reviewed Apr 27, 2018

View reviewed changes

hulpke force-pushed the ah/immediate branch from 0c54f7a to 824cd1b Compare April 28, 2018 23:14

gap-system deleted a comment from hulpke Apr 29, 2018

hulpke force-pushed the ah/immediate branch 4 times, most recently from cc93123 to 186ea11 Compare April 30, 2018 00:30

hulpke removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Apr 30, 2018

hulpke mentioned this pull request May 1, 2018

Partial fix and workaround for #2400 #2429

Closed

hulpke force-pushed the ah/immediate branch 2 times, most recently from 86e8b4f to f2d70e1 Compare May 1, 2018 15:18

fingolfin reviewed May 1, 2018

View reviewed changes

hulpke force-pushed the ah/immediate branch from f2d70e1 to c0466aa Compare May 1, 2018 20:42

hulpke force-pushed the ah/immediate branch from c0466aa to e804843 Compare May 9, 2018 21:44

hulpke removed the request for comments label May 16, 2018

hulpke mentioned this pull request May 16, 2018

Fix two bugs related to empty magmas #2452

Merged

fingolfin reviewed May 28, 2018

View reviewed changes

fingolfin approved these changes May 28, 2018

View reviewed changes

fingolfin mentioned this pull request May 30, 2018

HPC-GAP: sync coll.gd and type.g #2501

Merged

fingolfin mentioned this pull request Jun 6, 2018

Reduce impact of immediate methods: change some to implications, remove others, use custom SetSize method (rebased #2387) #2522

Merged

fingolfin closed this in #2522 Jun 7, 2018

fingolfin mentioned this pull request Jun 15, 2018

Regression in testextra/grpauto.tst (memory usage) #2377

Closed

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 28, 2018

		RightCosetCanonicalRepresentativeDeterminator);
		SetSize(d,Size(U)); # as own setter

Reduce impact of immediate methods to calculations with groups #2387

Reduce impact of immediate methods to calculations with groups #2387

Conversation

hulpke commented Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulpke commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulpke commented May 1, 2018 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulpke commented May 1, 2018

fingolfin commented May 4, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulpke commented Apr 22, 2018 •

edited

Loading

codecov bot commented Apr 23, 2018 •

edited

Loading

hulpke commented Apr 24, 2018 •

edited

Loading

hulpke commented May 1, 2018 •

edited

Loading