Minor performance improvements, code cleanup and very local fixes #2733
Conversation
hulpke
added
kind: bug
Codecov Report
@@ Coverage Diff @@
## master #2733 +/- ##
==========================================
- Coverage 75.87% 75.87% -0.01%
==========================================
Files 481 481
Lines 241305 241333 +28
==========================================
+ Hits 183089 183110 +21
- Misses 58216 58223 +7
|
hulpke
removed
the
do not comment yet
lib/grp.gi
Outdated
| r:=Number(a,IsZero); | ||
| a:=Filtered(a,x->not IsZero(x)); | ||
| if Length(a)=0 then return r; fi; | ||
| a:=List(Set(Factors(Product(a))),p->Number(a,x->x mod p=0)); |
There was a problem hiding this comment.
Multiplying prime powers together only to factor them again is wasteful. This could be avoided e.g. as follows:
a:=List(Set(a, SmallestRootInt), p->Number(a,x->x mod p=0));
There was a problem hiding this comment.
If this is considered nicer, I'll change it.
In practice this is a purely academic argument as we are talking about factorization into known primes, i.e. trial division.
There was a problem hiding this comment.
Any review remark I make of course just constitutes my personal opinion, and you are of course free to ignore it.
Anyway, all in all, over the past months I got a strong feeling that my remarks on your PRs are perceived more as nuisance than useful, and as "wasting your time". Therefore, in order to not waste both our time, I will refrain from reviewing your PRs for the time being, and leave it to others to take care of that.
lib/matrix.gd
Outdated
| @@ -1677,6 +1677,8 @@ DeclareGlobalFunction( "RandomMat" ); | |||
| ## returns a new random mutable <A>m</A><M>\times</M><A>m</A> matrix with integer | |||
| ## entries that is invertible over the integers. | |||
| ## Optionally, a random source <A>rs</A> can be supplied. | |||
| ## If the option <A>domain</A> is given, random seection is made from <A>domain</A>, otherwise | |||
|
Dear @fingolfin
Well, since proposed changes cannot be merged in without an approving review, any review that comments but does not approve is implicitly stating that these are issues without which the proposed contribution should be rejected.
I know that I do not react well to stylistic suggestions of changes. I'm sorry.
Then, why do we have the enforced review for every contribution? I fear that we have technical features of github dictate the way how we collaborate. |
I disagree with this interpretation of what a "review that comments but does not approve" means. It would ultimately imply that one must not comment on a PR, ever, unless to either approve it or reject it. So, if I spot a typo, I shouldn't point it out, unless I am willing and able to review the whole PR? That makes no sense to me. Esp. bigger PRs are hard to review in one go, so a partial review sometimes is the best I can afford with limited time and energy. In any case, this interpretation does not match my intentions at all. Indeed, if I reject a PR in its current form, I use the "request changes" option. If I think a PR could be merged in its current form (even if there are some changes I'd prefer to see), I'll approve it. If I neither "approve" nor "request changes", that can mean a lot of things: that I did not have time to review it all; that I do not feel informed enough to express an opinion either way; that I am conflicted about it; or something completely else. And based on past discussions with other GAP developers, many (most?) seem to have a rather similar view on this. But they can and should speak for themselves, of course. In the case of this particular PR, it's really that I did not have the energy to slog through all of it. There are tons of different, unrelated changes mingled up together in it. IMHO, if instead of this, it was split into several smaller commits, somebody likely would have reviewed most or even all of them already. As it is, I'd have to either review all of them (for which I don't have the energy, and quite frankly, also don't see why I should spend the effort to accommodate you, when I'd tell anybody else to split up the PR into multiple). Or I could either blindly rubber stamp it (but I don't like doing that); or request that you break it up (but based on past experience, I don't expect you to react favorably to this, so I didn't bother to do so). Nor do I want to reject it, as I have no actual concerns about it (by virtue of not having reviewed it). Thus, I am instead leaving it for somebody else to review.
Well, here we have a fundamental disagreement then. In my view, every change should be reviewed by at least one other party. From that view point, it does not matter if you are soliciting for reviews: any changes that goes in is reviewed, period. To avoid a misunderstanding, let me emphasize that I expressed that this is simply my personal view (although I believe that several other GAP developers share it), and that it's not me being a dictator blocking your PR out of a whim. You can raise this on the mailing list if you like, perhaps the GAP team can agree on a different approach.
This is not a github dictate; it's a setting we chose to activate. And which we can of course deactivate again, should we (as a collective) decide to do so. Feel free to bring this up on one of the mailing lists for discussion, too! Here's my view on it, though: We enforce reviews of every contribution to ensure a certain minimal quality level for them. E.g. that they are understandable, sufficiently explained/documented, accompanied by sensible tests, etc. To be frank, I usually have (mild!) concerns about that with your PRs (e.g. lack of tests -- and I am not talking about tests for performance improvements, where you explain that the tests you have take hours or days -- that's of course perfectly reasonable!; I am talking about tests that verify that e.g. a bug fix does actually fix a bug; or tests that verify that a given change does not break existing functionality). That said, so far I usually was happy to give you far more leeway than I'd give almost any other contributor with my reviews, because I of course know you are a master of your domain, and you make very valuable contributions. However, I learned that you do not appreciate the feedback I give on that work, and even complained to others about it (indirectly at least). Very well. I could write a lot about that, but in the end, I'll just accept it for the time being, and move on, and, as I wrote, not waste both our time with it then. But it's not my intention to block your PRs with that! Rather, I expect others to review them. There is no magical law that says that I am the only one who can review your PRs. That said, we do indeed have a shortage of people who actively review PRs. That is a problem. And perhaps it means we should stop requiring reviews again (although personally I'd consider it a shame). Once again, this is of course something that can be discussed, ideally on the mailing list. |
|
Perhaps would be good to discuss on a mailing list and formulate a policy and explain the reasoning behind some decisions (like above). I would also consider stopping requiring reviews to be a shame. Reviewing is hard, and it takes time, but it's useful when others look at the code, not only because they may have suggestions, but also because they will learn something by looking at it. And we do manage to merge PRs - for a long time, we keep the number of concurrently open PRs below 50, and we have 1831 closed PR by now! OTOH, the larger is the change, the harder is to review it. @hulpke I looked at your PR when it appeared, and seen that the earliest commit sits there since May. It's good that your changes are kept in individual commits, but wouldn't be more efficient to go further and keep unrelated changes in separate PRs, and submit them as soon as they are ready? In particular, we plan to branch off stable-4.10 on September 1st, and seems unlikely that this PR will go there (cherry-picking later will require extra work), while having some of these commits merged earlier would help to that... |
|
I will try to write to the mailing list in the next days. I find the current setup deeply discouraging to contributions.
You can click on the individual commits (in the same time required as if they were several PR) and see the (often pathetically trivial) content.
If the main part of GAP development is the reviewing and wrapping process, then yes. Maybe even if these are deliberate changes that happen as main work.
There once was a policy that releases would aim to correct all known bugs. They would certainly include all bugfixes that had been made available. It is disheartening to see this go. |
| @@ -858,6 +861,10 @@ InstallTrueMethod( IsPolycyclicGroup, | |||
| ## | |||
| DeclareAttribute( "AbelianInvariants", IsGroup ); | |||
|
|
|||
| # minimal number of generators for abelianization. Used internally to check | |||
| # whether it is worth to attempt to reduce generator number | |||
| DeclareAttribute( "AbelianRank", IsGroup ); | |||
There was a problem hiding this comment.
[Question] would it be useful to document this attribute?
There was a problem hiding this comment.
My concern was that this could lead to confusion as AbelianInvariants will (in general) return a longer list by separating different primes, while this attribute gives the minimal generator number of
| if ValueOption("cheap")=true then return [];fi; | ||
| Info(InfoLattice,1,"MaxesAlmostSimple: Fallback to lattice"); | ||
| return MaxesByLattice(G); | ||
| InstallMethod(MaxesAlmostSimple,"permutation group",true,[IsPermGroup],0, |
There was a problem hiding this comment.
[Question] Do we have any guidance regarding writing the free text comment? Is "permutation group" ok here, of "for permutation group" would be better?
lib/matrix.gi
Outdated
| @@ -3651,8 +3651,13 @@ end ); | |||
| #F RandomUnimodularMat( [rs ,] <m> ) . . . . . . . random unimodular matrix | |||
| ## | |||
| InstallGlobalFunction( RandomUnimodularMat, function ( arg ) | |||
| local rs, m, mat, c, i, j, k, l, a, b, v, w, gcd; | |||
| local rs, m, mat, c, i, j, k, l, a, b, v, w, gcd,dom; | |||
There was a problem hiding this comment.
[Minor] Sorry for the nitpicking, but why there are spaces between all commas except the last one?
lib/matrix.gd
Outdated
| @@ -1677,6 +1677,8 @@ DeclareGlobalFunction( "RandomMat" ); | |||
| ## returns a new random mutable <A>m</A><M>\times</M><A>m</A> matrix with integer | |||
| ## entries that is invertible over the integers. | |||
| ## Optionally, a random source <A>rs</A> can be supplied. | |||
| ## If the option <A>domain</A> is given, random selection is made from <A>domain</A>, otherwise | |||
| ## from <A>Integers</A> | |||
| ## <Example><![CDATA[ | |||
There was a problem hiding this comment.
[Suggestion] I think it will be useful to have an example demonstrating how this option is used, and also indicate that in the Func element above. This will also make the usage of <A> less confusing.
There was a problem hiding this comment.
I added an example. This will require two iterations as the manual check will discover the appropriate random values.
There was a problem hiding this comment.
First, there is a typo domail. Second, what about
<Func Name="RandomUnimodularMat" Arg='[rs ,] m [:domain:=d]'/>
...
## If the option <C>domain</C> is given, random selection is made from the domain
## <A>d</A> (allowing for larger coefficients), otherwise the default domain will be <Ref Var="Integers"/>.
There was a problem hiding this comment.
Good example, which uncovers my misunderstanding. I thought that a domain should be a domain in a GAP sense (especially since Integers is a domain). But that's not necessary:
gap> IsDomain([-100..100]);
false
Maybe a different word/option name to avoid confusion? I guess d should be a collection for the code to work?
There was a problem hiding this comment.
The option is now called random for lack of a better word. (It is the set from which random numbers are chosen, matrix coeffs can be larger.
| @@ -780,6 +780,7 @@ InstallMethod( Order, | |||
| [ IsPerm ], | |||
| ORDER_PERM ); | |||
|
|
|||
|
|
|||
| ############################################################################# | |||
There was a problem hiding this comment.
[Minor] Accidental empty line pollutes changes history for this file.
| ds := DerivedSeriesOfGroup( G ); | ||
| return ds[ Length( ds ) ]; | ||
| fi; | ||
| # the normal closure actually seems to be faster than derived series, so |
There was a problem hiding this comment.
[Maybe] Remove the code instead of commenting it out, and describe in the comment that formerly there was some code here which was removed because of some reasons.
There was a problem hiding this comment.
I don't think I have run or analyzed enough examples to state categorically that the old (now commented out) code is clearly useless. Someone (well, myself) might stumble on this place again when looking at an inefficiency, and it might turn out that under certain circumstances the now commented-out code is in fact better. If it is immediately removed, it is hard to recover it from git.
lib/grpffmat.gi
Outdated
| @@ -125,11 +125,18 @@ end ); | |||
| ## | |||
| #M NiceMonomorphism( <ffe-mat-grp> ) | |||
| ## | |||
| FULLGLNICOCACHE:=[]; | |||
There was a problem hiding this comment.
[Question] This will certainly require attention in HPC-GAP. Is this code covered by test? Codecov is not accessible right now, so I can't check.
There was a problem hiding this comment.
AH, yes. This is triggered as soon as a permutation representation for GL (or a subgroup on the whole space) is calculated, that will be part of existing tests.
There was a problem hiding this comment.
PS: This list would not have to be consistent amongst different processors, as the homomorphism computed for a particular dimension/field will always be the same.
So if one processor caches and another not (yet), no harm is done. It is purely an efficiency improver to avoid recalculating homomorphisms if matrix groups are created repeatedly.
There was a problem hiding this comment.
Thanks for comment - then in HPC-GAP that could be a thread local variable, so each thread can assign a distinct value to it: http://www.gap-system.org/Manuals/doc/hpc/chap2.html#X7D93681D7B5E8DCD
|
It looks good to me - I suggest to look at least at the comment marked [Suggestion] because it will help to make documentation more clear. Also, before merging you need to do rebase and instead of two separate commits dealing with comments they should be incorporated into commits to which those changes belong. With |
|
@hulpke wrote
Of course we aim to correct all known bugs! But we do not want neverending release cycles, when we are waiting for some bugfixes to appear in the release branch, and thus delaying giving to the users access to all other bugfixes and new features that are already there. On the other hand, we tend to release often, so this PR has all chances to appear in GAP 4.11, and perhaps parts of it will be cheery-picked onto stable-4.10 after all... |
|
I also forgot to check commit messages (perhaps we do need a template for PRs with tickboxes for reviewers?) and would look better and match recommendations from here if they could be rewritten to at least something like and (also "gen. nr. arguments" could now be expanded). |
hulpke
force-pushed
the
additions
branch
2 times, most recently
from
a88f230
to
e6fa797
Compare
|
@hulpke just one more idea - we can avoid the necessity of choosing a name for the option for the domain in Since options stack is global, and there is no way to check when the option is misspelt, using variadic functions is IMHO more robust. |
|
@alex-konovalov
Thinking of the last option, maybe it actually should be an option for the |
|
@hulpke I tested this PR in Jenkins to run extended tests with all packages loaded etc. All went well except that there is one diff when all packages are loaded - this should be fixed before merging: |
|
@alex-konovalov |
|
@hulpke this is with current package-master.tar.gz linked from https://github.com/gap-system/gap-distribution. There is no backtrace for this error in the test log, and in a fresh checkout of your branch with the said package archive I also can't reproduce it. Noticed that you have updated this PR - maybe that's why? Also, while |
|
@alex-konovalov |
|
I just tried to reproduce this weird error in That said, it's still somewhat worrying. I have a hard time imagining any reason that would cause such a test failure "randomly". If this failure happened on that Mac OS X test server in St Andrews, I'd suspect a HW fault, but on kovacs...? |
|
I am convinced that this is a genuine error. @hulpke if the error in the package test on travis was about missing symbol "atexit", please ignore it - that was due to some Travis irregularities. We have now disabled building packages that trigger the problem (cf. #2768), and @fingolfin will hopefully fix this properly in #2759. I have rerun it today in Jenkins, and it happened again - both on kovacs and macOS nodes, and both in 32- and 64-bit builds. I am now running it manually on kovacs, trying to catch some more details. |
|
Update: reproduced interactively in the workspace of the failed Jenkins build:
I have modified Also, I tried to exclude tests from |
|
@alex-konovalov |
|
@alex-konovalov |
|
Thanks @hulpke - I will rerun the tests now. |
|
@hulpke now teststandard passes! Next, do you plan to rebase after I've run the tests, and use the two last commits to fix some other commits in this branch? |
Remove extra test that is not worth doing. Immediately add conjugate when forming normal closure.
as this makes a Core computation quicker. This avoids slowdowns in particular cases such as gap-system#2683 This fixes gap-system#2683 Includes necessary manual example changes.
This permits packages to overload new methods without touching existing code. Also have simple group imply almost simple group.
Used to clean up generator number arguments in multiple places.
Also add first cheap cycle structure test to be conjugator automorphism. This fixes gap-system#2724
|
@alex-konovalov |
|
For the record, I've compared performance in this PR and in nightly builds yesterday.
Linux:
macOS:
Linux:
macOS:
Jenkins nightly build #467 (Sep 6, 2018 11:15:11 PM) was based on revision fe8f60c |
|
Thanks @hulpke ! |
olexandr-konovalov
added
the
release notes: to be added
fingolfin
added
release notes: added
Most of this is changes that are entirely "under the hood" for a user and improve performance in particular (often too large to allow for specific test examples) cases I observed, or that clean up ugly code. It is not worth mentioning it specifically in a release announcement.
The utility function
MaxesAlmostSimpleis made an operation to allow for future packages to overload (and thus improve the maximal subgroups code).The only user-visible changes are:
SubgroupsSolvableGroup.