FIX/ENHANCE: ImageKernelBlockHomomorphism memory and performance #2025

hulpke · 2017-12-14T17:28:30Z

in ImageKernelBlockHomomorphism do not add superfluous generators.
This otherwise can cause bad memory problems in large degrees with few
blocks (observed by Thomas).
Also if there are few large blocks, use orbit on blocks, together with test for redundant Schreier generators, rather than running through all points in block.

codecov · 2017-12-14T18:10:51Z

Codecov Report

Merging #2025 into master will increase coverage by <.01%.
The diff coverage is 71.25%.

@@            Coverage Diff             @@
##           master    #2025      +/-   ##
==========================================
+ Coverage   66.02%   66.02%   +<.01%     
==========================================
  Files         898      898              
  Lines      273285   273311      +26     
  Branches    12773    12773              
==========================================
+ Hits       180429   180456      +27     
+ Misses      90035    90033       -2     
- Partials     2821     2822       +1

Impacted Files	Coverage Δ
lib/ghomperm.gi	`90.63% <71.25%> (+0.19%)`	⬆️
src/vecffe.c	`62.85% <0%> (-1.76%)`	⬇️
src/weakptr.c	`78.92% <0%> (-1.54%)`	⬇️
src/blister.c	`75.09% <0%> (-0.73%)`	⬇️
src/sortbase.h	`84.93% <0%> (-0.61%)`	⬇️
src/objfgelm.c	`64.33% <0%> (-0.29%)`	⬇️
hpcgap/lib/hpc/stdtasks.g	`38.61% <0%> (-0.26%)`	⬇️
src/stringobj.c	`78.78% <0%> (-0.24%)`	⬇️
src/hpc/thread.c	`46.01% <0%> (-0.2%)`	⬇️
lib/stbcrand.gi	`90.36% <0%> (-0.2%)`	⬇️
... and 14 more

fingolfin · 2017-12-15T19:31:47Z

lib/ghomperm.gi

+                  img:=D[orb[j]][1]^k;
+                  p:=PositionProperty(D,x->img in x);
+                  if p=fail then Error("block are not well");fi;
+                  if not p in orb then


So this is yet another ad-hoc orbit algo implementation.

Are the orbits going to be very short? If not, wouldn't it make more sense to use a dictionary here, instead of letting GAP perform a linear search through orb.

I thought about dictionaries, but it is an orbit on blocks, and I'm always mapping just one point (and then find the block containing the point).
[Just realize that I should be able to remove PositionProperty and instead use the stored inverse list -- will do so to make the code better though practical effect will probably be neglegible.]

As for length, the orbit is at most of length |D|, but the condition requires that the permutation degree |B|*|D| is at least |D|^3, that is the orbit is at most third root of permutation degree.
(Thomas' example that triggered is was degree 150k and action on 9 blocks of size 17k each, this change sped up a calculation including other tasks by a factor of 10)

What might be good is to be more careful still with `AddGenerators', but at this point before a release I don't want to do tricky business whose function is not completely clear.

fingolfin

I have some nitpicks on the commit structure, and one on code formatting, but all in all this seems to be a clear improvement.

Oh, of course a test case would be great, too, but I am guessing it is difficult to provide one.

fingolfin · 2017-12-17T20:17:12Z

lib/ghomperm.gi

+##
+InstallMethod( KernelOfMultiplicativeGeneralMapping,"blocks homomorphism",
+    true,
+    [ IsBlocksHomomorphism ], 0,


So the first commit in this PR moves this method around (why), and also changes its rank to 200. The second commit changes the rank back to 0. Neither commit mentions any of this, though. What's going on?

Also, the first and third commit kind of are invisible because the code they touch is completely rewritten by the other commits. All in all, perhaps this PR should be squashed into a single commit?

The moving was to have the kernel method near the function that does the work. The ranking change was debugging code that should not have survived, and then was kicked out. I thought I had this commit already rebased away.
(Part of the reason for the multi-commits was to synchronize work and home.)
Anyhow, I can squash it all together before merging.

fingolfin · 2017-12-17T20:21:08Z

lib/ghomperm.gi

+              while j<=Length(orb) do
+                for k in S.generators do
+                  img:=D[orb[j]][1]^k;
+		  p:=hom!.reps[img];


Could you please avoid using tabs for indentation in this line?

They are a hello from my editor. I'll fix it.

PS: Is there a problem if I simply replace all tabs?

@hulpke I would recommend to replace ALL tabs only in separate commits (not necessarily in this PR). Otherwise, diffs will be very lengthy and it will be VERY hard to review this PR.

@alex-konovalov
The tab changes make up less than 1/3 of the overall PR, but I kept it a separate commit so it can be thrown out again.

hulpke · 2017-12-17T22:07:19Z

@fingolfin
I'm happy to provide an example, but it runs a couple of minutes (and without the change takes 20GB and half an hour). Can this go in the test files? (and which one would be appropriate)

olexandr-konovalov · 2017-12-18T00:18:56Z

@hulpke 2-minutes long example which is tested as part of this PR sounds OK with me. We can move it to testextra, to remove from Travis tests, later, but for now this will allow to test it in several settings, what's very useful.

The old code added every generator found with `AddGeneratorsExtendSchreierTree'. This caused the storage of lots of generators and subsequent memory issues. Also, if there are few large blocks, Butler's block homomorphism code is inefficient and it needs to run through all points in one block. In this case an ordinary orbit calculation is far more efficient. Together these issues did cause severe problems in large degrees with few blocks (observed by Thomas). It it possible that it also rectifies some of the observed problems in larger degree. What one could still do is to use random subproducts instead of testing all Schreier generators, but this is not the right time to do so.

hulpke · 2017-12-18T01:10:56Z

@alex-konovalov
Ive added an example, it takes about 4 minutes.

olexandr-konovalov · 2017-12-19T15:01:28Z

I've assigned this to GAP 4.9.1 milestone. @hulpke you will need to edit the PR so that it will be submitted to stable-4.9 branch (see https://github.com/blog/2224-change-the-base-branch-of-a-pull-request )

olexandr-konovalov · 2017-12-19T23:08:54Z

@hulpke could it be that because of that now when all packages are loaded I am observing this:

########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-master-test/GAPCOPT\
S/64build/GAPTARGET/standard/label/kovacs/GAP-master-snapshot/tst/testextra/gr\
pauto.tst:105
# Input is:
IsomorphismGroups(G,PcGroupCode(CodePcGroup(G),Size(G)))=fail;
# Expected output:
false
# But found:
Error, reached the pre-set memory limit
(change it with the -o command line option)
########

fingolfin · 2017-12-19T23:13:45Z

Was this merged intentionally?!?

olexandr-konovalov · 2017-12-19T23:14:47Z

@hulpke I am afraid that there is no evidence that c509e8b passes the tests. As I have said above, I was asking for "2-minutes long example which is tested as part of this PR" and tests in textextra are not, because of resource limitations. I have a way to test critical PRs in Jenkins before merging, so it would be useful to let me know that this PR has been updated and tests added, so that I will be able to check.

olexandr-konovalov · 2017-12-19T23:25:36Z

@hulpke also, note that testextra/grpperm.tst passes with additions made in this PR, but grpauto.tst is the one which fails.

hulpke · 2017-12-20T15:44:59Z

@fingolfin Yes, it was merged intentionally (before @alex-konovalov sent his last bunch of remarks)

The PR was approved
The automatic examples and the examples I tried all succeeded
4.9 was split off so that merging would not affect this release (and there is a patch
All issues that had been pointed out were addressed and I did not heard anything further for over a day.
I must admit that it is still not clear to me after various changes what the merge process is. If mergiung should only be done by a fixed set of people I'm happy to sit on my hands in the future, but that was not clear to me.

So what now:
Will someone else unmerge? Should I unmerge?

hulpke · 2017-12-20T20:51:47Z

Dear @alex-konovalov

there is no evidence that c509e8b passes the tests

Do you want to say it does not pass, or is the issue genuinely that of evidence? It would be helpful to have in an obvious place (say the etc directory) a description of what tests are supposed to pass and with what parameters (memory, assertion level etc.) If you want to run tests yourself before any merge let me know and how I should request this -- I had send you a message on this page after adding the test that it had been built in.

I have tested grpauto.tst, on my machine. It runs (with minimum packages) in under 1 GB with assertions turned on. I also tested crisp which is the one obvious candidate to have alternative methods that could be involved, but to no effect.
If you can point out which package causes the problem you observed I'd be happy to investigate.

Finally, about the tests: This is (as will be future changes, e.g. for socle) an issue that can only happen in large degree (100k+), implying not only that it runs for longer but also that such an example needs to be built in first place to avoid having to put thousands of lines with permutations in the test file, taking time for construction.
(Indeed the fix is to not follow only the description in Ákos' book, but to treat this situation special. )
I am doubtful one can fit such a test in 2 minutes. Apparently it does not fit in our current test setup and we need to decide whether we want such tests and how to treat them.

As I know that travis is time critical I put the extra test in in testextra, as it is a relatively straightforward code in the library it is unlikely to need the various different system architectures.

So, let me know what you would want me to do and I will try to do so.

olexandr-konovalov · 2017-12-20T23:35:21Z

@hulpke what I meant was that while commit message at c509e8b says "merged now as all tests work fine ...", the new test has been added to testextra directory which is not included into Travis CI tests - so if Travis tests work fine in this case, one can not conclude that your change to testextra/grpperm.tst has actually been exercised by Travis tests.

I will write more about tests later. In principle, it's easy to run make teststandard which will run with fixed parameters like memory and assertion level, and two different settings for packages, but then I understand that it's not practical for developers to wait until it will complete as it may take more than an hour. Here is exactly why I am offering testing some PRs with Jenkins prior to their merging.

fingolfin · 2017-12-20T23:41:12Z

@hulpke just to clarify: I don't think there is need to unmerge it (at least not urgently -- I am hopeful this can simply be resolved via some additional tweaks) . Nor did I mean to imply that merging this was bad, sorry for my bad wording. I simply was surprised that it was merged suddenly, because just before I noticed that I saw that PR against stable-4.9 with the same (?) content.

olexandr-konovalov · 2017-12-21T00:21:38Z

This night's run is a bit different: teststandard passes on linux and fails on mac. Incidentally, new Semigroups release fails to build on LInux (semigroups/Semigroups#431). Could this be related, @james-d-mitchell ?

hulpke · 2017-12-21T17:35:58Z

With this PR being closed, I think I'll move discussion to #2035 (which is so far the same commit)

olexandr-konovalov · 2018-01-24T14:50:28Z

OK, then I've changed milestone for this issue from 4.9.1 to 4.10.0, while #2035 has correct 4.9.1 milestone.

olexandr-konovalov · 2018-01-29T20:46:56Z

Added "not for release notes" label since I expect #2035 to be listed in release notes for 4.9.1.

hulpke added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: performance bugs or enhancements related to performance (improvements or regressions) labels Dec 14, 2017

hulpke changed the title ~~FIX: ImageKernelBlockHomomorphism only adds new generators~~ FIX/ENHANCE: ImageKernelBlockHomomorphism memory and performance Dec 14, 2017

hulpke force-pushed the additions branch 2 times, most recently from 4fe5de0 to f0b2db5 Compare December 15, 2017 03:21

fingolfin reviewed Dec 15, 2017

View reviewed changes

fingolfin approved these changes Dec 17, 2017

View reviewed changes

hulpke added 2 commits December 17, 2017 17:38

Replaced tabs with blanks to avoid indentation issues.

d67beef

hulpke force-pushed the additions branch from 1b2c4f1 to d67beef Compare December 18, 2017 00:44

Added test file

e29ed34

olexandr-konovalov added this to the GAP 4.9.1 milestone Dec 19, 2017

hulpke mentioned this pull request Dec 19, 2017

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

Closed

hulpke merged commit c509e8b into gap-system:master Dec 19, 2017

olexandr-konovalov mentioned this pull request Dec 21, 2017

Build fails on Linux semigroups/Semigroups#431

Closed

olexandr-konovalov modified the milestones: GAP 4.9.1, GAP 4.10.0 Jan 24, 2018

olexandr-konovalov added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 29, 2018

FIX/ENHANCE: ImageKernelBlockHomomorphism memory and performance #2025

FIX/ENHANCE: ImageKernelBlockHomomorphism memory and performance #2025

Uh oh!

Conversation

hulpke commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulpke Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulpke Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulpke commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olexandr-konovalov commented Dec 18, 2017

Uh oh!

hulpke commented Dec 18, 2017

Uh oh!

olexandr-konovalov commented Dec 19, 2017

Uh oh!

olexandr-konovalov commented Dec 19, 2017

Uh oh!

fingolfin commented Dec 19, 2017

Uh oh!

olexandr-konovalov commented Dec 19, 2017

Uh oh!

olexandr-konovalov commented Dec 19, 2017

Uh oh!

hulpke commented Dec 20, 2017

Uh oh!

hulpke commented Dec 20, 2017

Uh oh!

olexandr-konovalov commented Dec 20, 2017

Uh oh!

fingolfin commented Dec 20, 2017

Uh oh!

olexandr-konovalov commented Dec 21, 2017

Uh oh!

hulpke commented Dec 21, 2017

Uh oh!

olexandr-konovalov commented Jan 24, 2018

Uh oh!

olexandr-konovalov commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hulpke commented Dec 14, 2017 •

edited

Loading

codecov bot commented Dec 14, 2017 •

edited

Loading

hulpke Dec 18, 2017 •

edited

Loading

hulpke Dec 17, 2017 •

edited

Loading

hulpke commented Dec 17, 2017 •

edited

Loading