Ongoing improvements that do not need to make 4.9 #2057

hulpke · 2018-01-01T16:35:35Z

Better behavior if FpGroups are used for serious calculations (since some users seem to love word expressions).
Replace remaining FactInt calls.
Improvent to heuristics for small matrix groups.
Improved performance to GQuotients if there are many maps and more than two generators.

fingolfin

Looks good in principle to me, just some quirks in the commits?
Also, what's with that "REBASE: ..." commit, is that meant to be squashed into the first commit of this PR or what?

fingolfin · 2018-01-03T03:06:49Z

lib/grpfp.gi

@@ -3720,7 +3720,8 @@ local fgens,grels,max,gens,t,Attempt;
  t:=MostFrequentGeneratorFpGroup(G);
  gens:=Concatenation([t,
    #pseudorandom element - try if it works
-    Product([1..Random([2,3])],i->Random(gens)^Random([1,-1]))],
+    PseudoRandom(G:radius:=Random(2,3))],
+    #Product([1..Random([2,3])],i->Random(gens)^Random([1,-1]))],


This change is introduced by the commit adding ProcessEpimorphismToNewFpGroup -- an accident?

I spotted this while going over code that uses homomorphisms with fp groups. It seemed to small a change to deserve its own commit. (The PseudoRandom is cleaner than the `Random(gens)^Random([-1,1]) hack.)

OK, so then, remove the old code, and mention it in the commit message?

fingolfin · 2018-01-03T03:07:05Z

lib/grpfp.gi

+  #  w:=Factorization(k,w);
+  #  Print("wc=",w,"\n");
+  #  return UnderlyingElement(w);
+  #end;


Also this change is introduced by the commit adding ProcessEpimorphismToNewFpGroup -- another accident?

That is (commented out) debugging code in case an error arises later. I'll look before merging whether it is worth keeping.

OK, fair enough

fingolfin · 2018-01-03T03:08:34Z

tst/testinstall/semigrp.tst

@@ -477,7 +477,7 @@ true
 # test the methods for Random
 gap> S := FreeSemigroup(3);;
 gap> Random(S);
-s3*s1
+s1*s2


This test change perhaps was made necessary by those other accidental changes?

I don't think its the accidental changes but that an fp group created earlier from a finite group now immediately knew a faithful rep. and did not have to construct it.

In any case, tests for explicit results of Random should be expected to be highly unstable.

fingolfin · 2018-01-03T03:11:54Z

lib/ghomfp.gi

+  s:=SubgroupNC(s,mapi[1]);
+  fam:=FamilyObj(One(r));
+  fas:=FamilyObj(One(s));
+  if IsPermCollection(s) or IsMatrixCollection(s) 


Any particular reason you are not using IsPermGroup(s) or IsMatrixGroup(s) here? There should be no functional difference, but IMHO it conveys the intention of the code more naturally.

The reason for the category test was that I was thinking in the context of CanEasily.... It could as well be the Group test.

fingolfin · 2018-01-03T03:12:13Z

lib/ghomfp.gi

+    return; # the method does not apply here. One could try to deal with the
+    #extra generators separately, but that is too much work for what is
+    #intended as a minor hint.
+    Error("fp generators?");


Remove that Error?

fingolfin · 2018-01-03T03:16:04Z

lib/ghomfp.gi

+  if IsPermCollection(s) or IsMatrixCollection(s) 
+     or IsPcGroup(s) or CanEasilyCompareElements(s) then
+    #fpf:=InverseGeneralMapping(hom);
+    fpf:=GroupHomomorphismByImagesNC(r,s,mapi[2],mapi[1]);


Why not InverseGeneralMapping? Naively, that would seem more attractive: easier to understand the intent of the code that way, and of course the inverse is cached. Are there perhaps some performance concerns in certain cases? If so, it would be nice if there was a comment explaining what they are (also, perhaps we can resolve them?)

What it actually should be is the co-restricted inverse (or the inverse of the source-restriction. This is iffy now, so I explicitly make a new map to guarantee that there is no larger gropup somewhere hidden.

I see. That would be perfect for a comment; anybody looking at that code again in the future might wonder the same

fingolfin · 2018-01-03T03:17:54Z

lib/ghomfp.gi

+local s,r,fam,fas,fpf,mapi;
+  if not (HasIsSurjective(hom) and IsSurjective(hom)) then
+    Info(InfoWarning,1,"ill created epimorphismfp");
+    return; # hom might be ill defined.


Why is this only a warning? Shouldn't it be an error?

That depends on whether the function is considered internal (what I believe) or user.

My understanding was that all cases in which I call the function at this point have a homomorphism that will not trigger this warning, but there is no need to make it stop the calculation (there is a minor, probably nonexistent, efficiency pay.) Basically this is a warning that should never be triggered by the existing code, but could be triggered in the future if other code creating homomorphisms changes.

Makes sense, thanks.

fingolfin · 2018-01-03T03:18:08Z

lib/ghomfp.gi

+function(hom)
+local s,r,fam,fas,fpf,mapi;
+  if not (HasIsSurjective(hom) and IsSurjective(hom)) then
+    Info(InfoWarning,1,"ill created epimorphismfp");


"ill created" ? Perhaps "ill defined"?

This review was based on an earlier version of the PR, which since then was heavily modified

fingolfin · 2018-02-17T12:55:49Z

@hulpke This has a conflict now, which prevents it from merging.

Meta-question: So when is this "ready for merge"? I reviewed and approved this PR at some point, but since then it underwent major changes, making my "approval" worthless.

All in all, I find this approach of amassing changes in a single PR and then merging it all in one go problematic: it makes the PR a moving target for review; and it makes it unclear when to merge it. It also causes extra work when creating release notes, as a lot of (often unrelated) changes are grouped into a single PR.

E.g. the FactorsInt changes could have been merged a few weeks ago, and would have been tested since then by everybody working with the GAP master branch. If they cause any regressions, we might have already noticed and resolve them. As long as that change is stuck in this unmerged PR, this won't happen. At the same time, that single commit touches tons of files, cluttering up the diff for this PR, making a global review harder (one can still review commit-by-commit, though).

I am not completely sure why you work this way. I assume part is convenience of having all your fixes immediately at hand. That of course explains why one would want to keep those fixes on a single "work branch" -- but doing that (something I also sometimes do) does not make it necessary to put all commits in a single PR (and vice versa).

Instead, what I do, is to put separate changes/fixes into separate branches, and then into separate PRs. While locally, I may have an integration branch, where all my various bugfix branches are merged together. The only time this gets annoying is when one has to update a fix branch, and then has to update the integration branch. But there are tools to automate this. E.g. I use git reintegrate from https://github.com/felipec/git-reintegrate

hulpke · 2018-02-22T20:18:42Z

@fingolfin
I have rebased and the conflict is gone.

As for your meta-question, I've come from the old setup of CVS and it took a while to be comfortable with git in a way the did not cause issues for others, nor imposed a higher administrative cost on me.

What I settled is that I have my own repository copy that updates from GAP (master and current stable branch), and I have a work branch for myself (that gets updated from time to time to master.

To move things back into the repository, in the past I had just moved them into the (equivalent of master). This was considered as inappropriate and also missed the automatic tests. I thus have two branches (additions and fixes) into which I put my changes (through pull requests) back to master, respectively stable. (That is, I do the fix in my work branch and then cherry pick into (e.g.) additions.)

What I like about this setup is that it deals efficiently with changes that are not my current research interest, but which I have done as the person who caused the problem in the first place or knows the code best, often in response to a bug or a request (such as the small dimension matrix groups, or the issues with automorphism groups of fp groups).

I want to be able to take these such quickly and put them into a processing queue that lets me forget about the issue and go quickly back to my own work, while ensuring that the changes eventually go back to the production system. Making a separate branch and PR for each such change takes longer and also forces me to monitor multiple PRs to see that they eventually have gone in.

This setup predates newer changes (such as the now mandated reviews) and assumes that a merge will happen within a few days, cleaning the queue. (It still is not clear to me who should merge -- at times nobody did so, and when I finally merged I was told this was wrong.)

This setup predates recent changes (such as the mandated reviews, or requests to not merge into release-ready branches. I understand why this is happening, but for issues that I feel obliged to do, I am looking rather for a "Babyklappe" than for opportunity (or obligation) to discuss a minor fix in a global context or alternative options.

I'm perfectly happy to try another model that does not require more attention or time. If there is a guarantee that someone will take care of stale PR's I can put subject specific changes into different PRs. What I don't want to do is to have to go back a few weeks later and see whether changes I proposed were in fact incorporated or might still be hanging or have been dropped.

(This all affect such small fixes. I am prefectly happy to work through specific branches if it is substantial new material.)

fingolfin

Sorry, I missed that this PR had been updated and a comment being added :-(.

Looks OK now to me; only one tiny question on an operation vs. attribute -- but this could also be addressed in a future PR, if at all. I.e. feel free to just merge now.

fingolfin · 2018-03-06T21:37:49Z

Now to reply to your last comment, in brief: I am happy to promise to handle small, self-contained PRs quickly! (And I am hopeful that @alex-konovalov, @ChrisJefferson, and @markuspf will do so, too, once they are not on strike anymore). If I don't then usually because I missed the notification (on some days, there are just to many, and something slips through -- I usually try to scan through open PRs from time to time to catch anything I missed, but for various reasons this failed for this PR and also #2035.

Anyway: I totally get what you are doing with your "additions" and "fixes" branches, from the developers perspective

The problem I have, as a reviewer, is dealing with big PRs, which mix lots of things together, and also get upgraded over time with new major changes, i.e., which are a shifting target. And this PR has been that, from my perspective. There are multiple changes in it I'd have merged weeks ago if they had been in their own PR, but there were new things being added, and it was not clear to me when the PR was supposed to "ready for review and merge".

To work with PRs, the ideal PRs (from the perspective of a reviewer, not necessarily from that of a submitter ;-) ) are those which are focused on one subject and don't change once submitted (other than to address review comments and perhaps bugs found by the submitter). Or alternatively PRs which contains multiple disparate minor changes (bug fixes and so on), but not too many, and each of them not too big.

So if you are willing to try smaller, more focused PRs, that'd be great. Perhaps we could also work out something with your current "additions" and "fixes" branches, though the best I can think of is somebody else doing it for you (i.e. you make a big PR, somebody else cherry-picks parts into separat PRs, those get reviewed and merged, you rebase, rince and repeat). But then somebody has to have the time and energy for that, adding a new bottleneck...

I kind of wish we could discuss this in person, because I find it rather difficult to write about this clearly and w/o writing far too much which nobody has time to read anyway ;-).

Anyway, I think this PR is good to go, which is why I approved it prior to writing this :-).

While no sane person should try to calculate automorphism groups in the Fp representation, this commit changes: - Automorphisms of abelian fp groups still represent on free generators - Permrep for automorphism group of Fp does not attempt to be clever. Both together Fixes gap-system#2010

with `IsomorphismFpGroup`, store the *inverse* of this isomorphism as a known faithful map to a better behaved group, thus avoiding such a searched renewed if any "finite group" calculation is done in the FpGroup (not that this is advisable, but users do it). Ditto translate a known faithful map after calling `(Isomorphism)SimplifiedFpGroup`. This is suggested by gap-system#2020 also bail out in complicated case instead of trying hard The output of a `Random` call in test file has changed. REBASE: Comment cleanup

Changes to manual examples that list presentations obtained through random REBASE: Further manual examples

This way better factorizations methods in packages will get used. (Calls in the recursive implementation of FactInt are kept.) Also replace some calls by `PrimePGroup` etc. (This is a revised version of earlier as there was already a merge with PrimePGroup.) This addresses gap-system#2087

for the unlikely case that a lot of tiny matrix groups are created. This resolves gap-system#2133

Also avoid pc isom in NormalizerParemtSA if the task is pathetic anyhow.

Kernel test: Use better fingerprint and do not force fp subgroup equality test, but always keep same direct product. If solvable, use automorphisms of image to reduce: Embed into holomorph as pc or perm group and search in holomorph Improved test whether abelian quotients map as necessary condition. REBASE: gapdoc .... REBASE: Another manual example changed. REBASE: Example

This resolves gap-system#2173

codecov · 2018-03-06T21:40:35Z

Codecov Report

Merging #2057 into master will increase coverage by <.01%.
The diff coverage is 81.62%.

@@            Coverage Diff             @@
##           master    #2057      +/-   ##
==========================================
+ Coverage   69.88%   69.89%   +<.01%     
==========================================
  Files         481      481              
  Lines      252990   253181     +191     
==========================================
+ Hits       176813   176956     +143     
- Misses      76177    76225      +48

Impacted Files	Coverage Δ
lib/pcgs.gi	`68.7% <0%> (ø)`	⬆️
lib/ffe.gi	`74.39% <0%> (ø)`	⬆️
lib/grpperm.gi	`86.43% <0%> (ø)`	⬆️
lib/tietze.gi	`76.11% <100%> (+0.87%)`	⬆️
lib/randiso2.gi	`72.28% <100%> (ø)`	⬆️
lib/fldabnum.gi	`85.98% <100%> (ø)`	⬆️
lib/grppcfp.gi	`67.55% <100%> (+0.05%)`	⬆️
lib/grpnice.gi	`84.49% <100%> (+0.12%)`	⬆️
lib/grpffmat.gi	`94.67% <100%> (+0.06%)`	⬆️
lib/matrix.gi	`77.99% <100%> (ø)`	⬆️
... and 36 more

hulpke added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature topic: performance bugs or enhancements related to performance (improvements or regressions) labels Jan 1, 2018

fingolfin previously approved these changes Jan 3, 2018

View reviewed changes

fingolfin reviewed Jan 3, 2018

View reviewed changes

hulpke force-pushed the additions branch from fa13853 to a9f7d73 Compare January 9, 2018 17:37

hulpke force-pushed the additions branch from 4a4d792 to 13bd41a Compare January 23, 2018 16:18

hulpke force-pushed the additions branch 3 times, most recently from 8f5eb8c to 5ac56ae Compare February 5, 2018 23:46

ChrisJefferson mentioned this pull request Feb 6, 2018

Deal with Random changing attributes of object #2165

Merged

hulpke force-pushed the additions branch 2 times, most recently from 214685c to fcdfe49 Compare February 12, 2018 14:56

hulpke force-pushed the additions branch from fcdfe49 to d4aac34 Compare February 22, 2018 18:17

fingolfin approved these changes Mar 6, 2018

View reviewed changes

fingolfin mentioned this pull request Mar 6, 2018

FIX/ENHANCE: Small fixes and corrections, staged for 4.9.1 #2035

Closed

hulpke added 9 commits March 6, 2018 14:39

ENHANCE: Use PseudoRandom with radius instead of ad-hoc construction.

625f11f

Changes to manual examples that list presentations obtained through random REBASE: Further manual examples

ENHANCE: Slight update of heuristic for matgroup niceo

96a1abb

for the unlikely case that a lot of tiny matrix groups are created. This resolves gap-system#2133

NEW: MinimalFaithfulPermutationDegree and basic method

53ea932

ENHANCE: Use TOM data also for subgroups of almost simple.

3608af5

Also avoid pc isom in NormalizerParemtSA if the task is pathetic anyhow.

DOC: SubgroupOfWholeGroupByCosetTable assumes table is standardized.

80c1855

This resolves gap-system#2173

hulpke force-pushed the additions branch from d4aac34 to 80c1855 Compare March 6, 2018 21:39

hulpke merged commit 8644421 into gap-system:master Mar 6, 2018

fingolfin mentioned this pull request Mar 7, 2018

FactorsInt in library #2087

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ongoing improvements that do not need to make 4.9 #2057

Ongoing improvements that do not need to make 4.9 #2057

hulpke commented Jan 1, 2018 •

edited

Loading

fingolfin left a comment

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

hulpke Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin Jan 3, 2018

fingolfin commented Feb 17, 2018

hulpke commented Feb 22, 2018

fingolfin left a comment

fingolfin commented Mar 6, 2018

codecov bot commented Mar 6, 2018 •

edited

Loading

Ongoing improvements that do not need to make 4.9 #2057

Ongoing improvements that do not need to make 4.9 #2057

Conversation

hulpke commented Jan 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

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

fingolfin commented Feb 17, 2018

hulpke commented Feb 22, 2018

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin commented Mar 6, 2018

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

hulpke commented Jan 1, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading