Change definition of IsPGroup to *not* require finiteness

[fingolfin]

This is kind of the opposite of PR #483 which changes the implementation (i.e. the actual filter) of IsPGroup to imply IsFinite, matching its documentation. This PR instead adjusts the documentation to also include infinite p-groups.

That means of course that IsPGroup should no longer imply IsNilpotent -- instead, IsFinite and IsPGroup is needed.

We discussed this briefly a few years ago, and IIRC the thought was that it's too risky to change this, as code might implicitly rely on IsPGroup "implying" IsFinite. Well, I audited all distributed packages and the GAP library, and surprisingly little code does. And virtually all of it just would run into an error if used on an infinite p-group.

The main reason to adjust code to explicitly check for IsFinite thus would be to get more helpful error messages... I performed various adjustments in this direction, which were merged as part of PR #1578. I also submitted patches to a few packages which improved code that seemed to implicitly rely on p-groups being finite, but in all cases, the only effect an infinite p-group should have would be a plain error, or perhaps a non-terminating program (e.g. trying to get an infinite generating set).

In the end, all that is left for this PR is to adjust the documentation!

[codecov]

Codecov Report

Merging #1545 into master will decrease coverage by 0.05%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #1545      +/-   ##
==========================================
- Coverage   63.12%   63.06%   -0.06%     
==========================================
  Files         967      970       +3     
  Lines      290318   292381    +2063     
  Branches    12746    12906     +160     
==========================================
+ Hits       183269   184398    +1129     
- Misses     104248   105177     +929     
- Partials     2801     2806       +5

Impacted Files	Coverage Δ
lib/pcgs.gi	68.7% <ø> (ø)	⬆️
hpcgap/lib/grppc.gi	50.4% <ø> (ø)	⬆️
lib/grppc.gi	71.13% <ø> (ø)	⬆️
grp/basicmat.gi	85.3% <100%> (ø)	⬆️
lib/pquot.gi	80.88% <100%> (ø)	⬆️
lib/grpfp.gi	63.94% <100%> (+0.02%)	⬆️
lib/grp.gi	84.25% <93.33%> (+0.07%)	⬆️
src/sysfiles.c	28.42% <0%> (-2.05%)	⬇️
lib/variable.g	74.19% <0%> (-1.62%)	⬇️
hpcgap/lib/variable.g	71.81% <0%> (-0.91%)	⬇️
... and 32 more

[stevelinton]

I'd make the documentation of IsPGroup more explicit about finiteness, especially as it a change. That is I would say directly that infinite p groups are allowed and that code which relies on properties like finiteness or nilpotence should require (or test) it explicitly.

[fingolfin]

I agree with @stevelinton , this should be made very explicit. I updated the documentation comment, please check if that's better, or please suggest further edits. I also split out most of the changes into PR #1578, which I hope is uncontroversial, and could be merged sooner, so that this PR can focus on the change in the meaning of IsPGroup.

[fingolfin]

Updated to match PR #1578 -- interestingly, the only extra change is a change to the IsPGroup documentation. No code change was necessary; in other words, while we documeneted IsPGroup as implying finiteness, the actual filter in fact did not. To prove this, consider this:

gap> RankFilter(IsPGroup);
45
gap> RankFilter(IsPGroup and IsFinite);
58

[olexandr-konovalov]

@fingolfin segmentation fault in Travis test of HPC-GAP...

[fingolfin]

@alex-konovalov well, not surprising, given that it does om Travis, too :-)

[fingolfin]

Specifically, this PR has not yet been updated to match #1578, so testing it is pointless for now.

[fingolfin]

I have now pushed a rebased version of this PR, but to clarify, I don't expect this to go into 4.9 so briefly before release.

I'd still like #1578 to be merged, though.

[wilfwilson]

I support this change.

[hungaborhorvath]

I support this change. In fact, if #1578 went into 4.9, then I suggest to move this into 4.9, as well. This is really the documentation of that change.

[fingolfin]

@hungaborhorvath I disagree. First off, this is a fairly major change (even though it only affects documentation), and we already released the first beta of 4.9 -- it is now way too later for putting in such a change. Secondly, I don't see how you justify that this PR here "is really the documentation of [PR #1578]". The only thing I can see in that PR that might justify this assertion is that it changed

InstallTrueMethod( IsPowerfulPGroup, IsPGroup and IsAbelian );
...
InstallTrueMethod( IsNilpotentGroup, IsGroup and IsPGroup );

to this, with IsFinite added in two places:

InstallTrueMethod( IsPowerfulPGroup, IsFinite and IsPGroup and IsAbelian );
...
InstallTrueMethod( IsNilpotentGroup, IsGroup and IsPGroup and IsFinite );

But if one keeps up the definition that IsPGroup implies IsFinite, then these changes had no mathematical effect. They of course do have a technical effect in that if one somehow creates a finite p-group that knows it is a p-group, but not that it is finite, then these automatic implications don't trigger. But that is at worst a source of a slow-down; it can not cause mathematically incorrect results. It does, however, indeed prepare the switch of the definition of IsPGroup to not imply IsFinite, that's true -- but preparing for a change is not the same as making that change... :-)

Perhaps there is something else in that PR to support your claim, if so, please point it out, then we can decide if it needs to be reverted in the stable-4.9 branch.

[hungaborhorvath]

@fingolfin I see #1578 as the technical implementation of the consequences of the decision to use the usual definition of p-groups which allows for infinite groups, as well. You even admitted that this was your goal all along, but of course you did it in a way so that it does not break anything. In my opinion this change was long overdue, and I very much welcomed it.

In fact, as far as I can see, gap 4.9 does not assume anymore that p-groups are finite. So we might as well say that p-groups are not assumed to be finite, anymore. I do not see this as much of a problem. 4.9 is still beta, and it is not like it was distributed very widely as a stable release. In fact, google has no idea about gap 4.9, and had I not been on the gap forum, I would not know about its existence or where to download it. I think it is much worse when a package gets released which is not compatible with the current stable gap version (which happened I think with 4.8.9 and Semigroups package, maybe, I do not remember exactly).

But anyway, if you feel this is too big a change to put it into the stable 4.9 release, I do not mind much. If anyone wants to define infinite p-groups, they will be able to do so. I just personally do not see the point of taking another 2-3 (4-5?) years on this decision (until 4.10 comes out) when the actual work is already done and will be released with 4.9.

[fingolfin]

@hungaborhorvath FYI, I recently learned from @Stefan-Kohl that one can already now define infinite p-groups in GAP (albeit of course not yet in the filter IsPGroup), via the fr package.

As to timeline: Our plan is to release 4.10 later this year. That it took 2 years from 4.8 to 4.9 is unfortunate, but was mostly because of several large projects (e.g. revised build system, integration of HPC-GAP) which delayed the release by a year, and also later on organizational problems with e.g. the creation of release notes (which sounds like a trivial thing, but really isn't)

[fingolfin]

Also, conversely: GAP managed to be usable for 20+ years with this definition for IsPGroup, so there is really no rush to change it now.

[hungaborhorvath]

@fingolfin As I said, I do not mind if this does not go into 4.9, it is just simply my preference. Anyway, I hope that the timeline on 4.10 can be kept.

[ssiccha]

This looks good to me.

[ssiccha]

Is there some statement in the release notes that people should keep an eye out for slow-downs if they don't tell GAP that a p-group they are using is finite? I could imagine some pretty big slowdowns if a generic algorithm is used instead of one suitable for nilpotent ones.