Skip to content
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.

Sign up for GitHub

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

Jump to bottom

performance enhancements in subgroup lattice #3384

Merged
merged 2 commits into from
Mar 28, 2019
Merged

performance enhancements in subgroup lattice #3384

merged 2 commits into from
Mar 28, 2019

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Mar 27, 2019

With this, GAP can calculate the subgroups of HNM4 on 1920 points -- before runs out of memory.

@hulpke
ENHANCE: Memory use in subgroup lattice, subspaces of layer
04e91d2 
Do not use `Closure` but create subgroup from scratch to avoid forcing
potentially memory-intensive (and unneeded) stabilier chains.

Use fixed pre-images for modulo pcgs elements t avoid redundant storing of
same elements.

Example (too large for routine test): Max 4 of HN on 1920 points,
ConjugacyClassesSubgroups.
@hulpke
ENHANCE: Option for perm group closure to get order of closure
0ac827a 
This can give speedup if closure is with normal subgroup.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 85.154% when pulling 0ac827a on hulpke:additions into bf9bec2 on gap-system:master.

wilfwilson
wilfwilson approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hulpke, I've had a look and tried to understand what's going on. The cache part makes sense to me. However I couldn't work out your rationale behind some other changes, so I've added a couple of questions in the hope that you can satisfy my genuine curiosity.

It's clear that you've not changed the mathematical correctness of the function, and I'll take your word for it that the performance is now better, so I'm happy to approve.

lib/grppclat.gi
else
a:=ClosureGroup(ker,List(i,j->PcElementByExponentsNC(pcgs,j)));
#a:=ClosureGroup(ker,List(i,j->pcelm(j)):knownClosureSize:=asz);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you replace the ClosureGroup with the SubgroupNC? Is it faster? And given the replacement, why have you kept the ClosureGroup line as a comment?

lib/grpperm.gi
@@ -736,6 +736,8 @@ BindGlobal("DoClosurePrmGp",function( G, gens, options )
fi;
od;
else
o:=ValueOption("knownClosureSize");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you intend for the knownClosureSize value option: if you're not using it in the code in the rest of the PR, and it's not documented, is there any point in it being there?

@DominikBernhardt DominikBernhardt added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 27, 2019
@wilfwilson wilfwilson added topic: performance bugs or enhancements related to performance (improvements or regressions) topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Mar 27, 2019
@hulpke
Copy link
Contributor Author

hulpke commented Mar 28, 2019

@wilfwilson This is a bit complicated:
The underlying problem, Closure(Sub)group taking significant time pops up in a number of solvable radical type calculations. In the case of subgroup lattice I now observed cases where it became so extreme that it required to be resolved.

The "knownClosureSize" was the first attempt, it helped, but not enough. I suspect there will be other cases where it should be used, which is the reason for keeping the code in.
The reason for keeping the commented line is that "morally" this should be a closure computation, not a new subgroup. I'm still slightly puzzled why the stabilizer chains computed by Closure take up so much memory, and I hope in the long run to have a "cleaner" fix, preferrably one that actually still can use Closure.

@wilfwilson
Copy link
Member

Thanks for the explanation 🙂 @hulpke

@wilfwilson wilfwilson merged commit 0ef46dc into gap-system:master Mar 28, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson approved these changes

@DominikBernhardt DominikBernhardt DominikBernhardt approved these changes

@wilfwilson wilfwilson wilfwilson approved these changes

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@hulpke @coveralls @wilfwilson @ChrisJefferson @DominikBernhardt @olexandr-konovalov