-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix and simplify method reordering for constructors #2815
Fix and simplify method reordering for constructors #2815
Conversation
For some reason, the method reordering code went through methods for constructors in reverse order compared to those for any other kind of operation. There doesn't seem to be a reason for that, and by getting rid of it, we remove a lot of code duplication and simplify the code overall. As a side effect, this fixes a regression, where reordering of constructors broke their relative order, because the alternate code for reordering constructors contained a bug in the code shifting the methods around. The result of that was the following: Without packages, we got this: gap> Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, ": ", r.info, "\n"); end); -59: SymmetricGroupCons: perm group with domain -59: SymmetricGroupCons: perm group with degree -64: SymmetricGroupCons: pc group with degree -66: SymmetricGroupCons: regular perm group with domain -66: SymmetricGroupCons: regular perm group with degree But after calling `LoadAllPackages()`, we got this; note how the methods are *not* ordered by descending rank anymore: gap> Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, ": ", r.info, "\n"); end); 9853: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA) -111: SymmetricGroupCons: for an LpGroup and a positive integer -117: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA) -154: SymmetricGroupCons: regular perm group with domain -154: SymmetricGroupCons: regular perm group with degree -152: SymmetricGroupCons: pc group with degree -150: SymmetricGroupCons: pcp group with degree -147: SymmetricGroupCons: perm group with domain -147: SymmetricGroupCons: perm group with degree This resulted in `SymmetricGroup(3)` running into an infinite recursion, and many other similar problems. With this patch applied, we get the expected result: Perform(MethodsOperation(SymmetricGroupCons,2), function(r) Print(r.rank, ": ", r.info, "\n"); end); 9853: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA) -111: SymmetricGroupCons: for an LpGroup and a positive integer -117: SymmetricGroupCons: for a partition of a ring into residue classes (RCWA) -147: SymmetricGroupCons: perm group with domain -147: SymmetricGroupCons: perm group with degree -150: SymmetricGroupCons: pcp group with degree -152: SymmetricGroupCons: pc group with degree -154: SymmetricGroupCons: regular perm group with domain -154: SymmetricGroupCons: regular perm
aaec695
to
872af26
Compare
for j in [1..n] do | ||
req := meths[base+1+j]; | ||
rank := rank+RankFilter(WITH_IMPS_FLAGS(req)); | ||
rank := rank + RankFilter(req); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that RankFilter
automatically calls WITH_IMPS_FLAGS
(and in general even WITH_HIDDEN_IMPS_FLAGS
), so there is no need for that WITH_IMPS_FLAGS
here (and removing it aligns the code here more closely to that in INSTALL_METHOD_FLAGS
)
This supposedly caused a lot of tests failures when more packages were loaded - see e.g. https://travis-ci.org/gap-system/gap-docker-master-testsuite/builds/428923967. Thanks @fingolfin - I hope this will fix it, will test. |
A brief update for now: I run overnight tests of this PR in Jenkins, it reduces number of diffs by half, to: 163 in teststandard, 161 in testinstall and 15 in testbugfix. |
@stevelinton is there a reason for iterating through constructors in reverse order that I missed? |
@stevelinton @markuspf @ChrisJefferson Right now, hundreds of tests are broken when all packages are loaded, due to issue #2818, and this PR fixes a large number of those, with PR #2841 fixing many additional ones. It'd be great if this PR could be reviewed and merged soon -- alternatively, just take my word and approve it (and I'll promise to help dealing with any fallout, should there be on. Of course @stevelinton is best equipped for a review, as he wrote the code in question. |
This also causes many of the failed pack tests visible on e.g. https://travis-ci.org/gap-system/gap-docker-pkg-tests-master |
I am looking at this now, I seem to remember that Constructors are ordered in the opposite order to methods, but I'll have a closer look. Might take me till tomorrow to finish this review though |
Constructors are ordered in the opposite order, that's right -- but this is already achieved by multiplying the rank of their first argument by -1. But then they are sorted by rank in ascending order, just like regular methods, as the examples in the issue description show. In fact, the old removed code also "sorted" this way, or rather "bubbled up/down" entries in the wrong plan; the only difference was that it started at the end and iterated towards the starts. Well, and that the code had a bug in it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me, with the disclaimer that I have never deeply looked at this part of GAP's code.
That's fair. I'll take responsibility then for the changes in here -- and hope that if there are issues with it discovered later on, the author of the original change (@stevelinton) which this PR tries to fix, will help resolve it. |
For some reason, the method reordering code added in PR #2773 went through methods for constructors in reverse order compared to those for any other kind of operation. There doesn't seem to be a reason for that, and by getting rid of it, we remove a lot of code duplication and simplify the code overall.
[For reviewing this PR, I recommend activating "Hide whitespace changes" in GitHub's "Diff settings", which reveals that it really only deletes a block of code and modifies the computation of
rank
in the remaining code -- and then adjusts the indentation of that remaining code.]As a side effect, this fixes a regression, where reordering of constructors
broke their relative order, because the alternate code for reordering
constructors contained a bug in the code shifting the methods around. The
result of that was the following: Without packages, we got this:
But after calling
LoadAllPackages()
, we got this; note how themethods are not ordered by descending rank anymore:
This resulted in
SymmetricGroup(3)
running into an infinite recursion, andmany other similar problems.
With this patch applied, we get the expected result: