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.

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

Speed up building stabchain from base and strong generators #4331

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Mar 23, 2021

When we already have a base + strong generating set, the function StabChainBaseStrongGenerators turns this into a GAP stabilizer chain. This function scaled surprisingly badly, because it used existing functions for adding to a stabilizer chain which have "O(n^2) in number of generator" loops.

Long term it would be good to clean those up, but in this case, where we have a list of strong generators, we can just construct the stabilizer chain.

A brief benchmark:

sc := StabChainBaseStrongGenerators([1..1000], List([1,3..999], x -> (x,x+1)));; is sped up from 48 seconds to 45 milliseconds (that's not a unit mistake, it is 1000 times faster).

I test this with the existing tests for stabiliser chains, which use the fact that stabiliser chains are used for enumerating groups.

This includes another fix (which I could pull out) -- the function PlainListCopy didn't actually produce a plain list for types like ranges, which I needed to fix as the internal AGEST only accepts PLISTs.

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.

Unsurprisingly, touching this code has had consequences! 🙈 I hope they're not too hard to iron out.

Although I'm not a stickler for this kind of stuff, I think it would be best if the PlainListCopy stuff was in a separate PR. But I don't mind. In any case, although PlainListCopy (and PlainListCopyOp) have some documentation in the lib/list.gd file, this documentation isn't linked into the manual, but in my opinion that would be helpful (probably in Section 21.24 Plain Lists, at the very end of Chapter 21: Lists). If you wouldn't mind adding that, I'd appreciate it.

I certainly support the main change you've made to StabChainBaseStrongGenerators, especially given the performance problems we were encountering yesterday. My main suggestion is that you should include your 1000-fold speedup example in a test file (perhaps made even bigger) to make a regression super obvious.

Note that this example includes a redundant base. Although that's not forbidden, maybe you'd prefer to give an irredundant base (i.e. use the same range in both places):

sc := StabChainBaseStrongGenerators([1,3..999], List([1,3..999], x -> (x,x+1)), ());;

Fingers crossed the tests pass this time.

lib/list.gi Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

The "fix PlainListCopy" part of this PR is now #4332 (this PR contains that PR, because it's needed for tests to pass)

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.

I've thought about this a fair amount, and I'm happy to approve. It would be nice if you include my suggestion below (basically your benchmark example, increased a little).

This should be merged after #4332.

tst/testinstall/stabchain.tst Show resolved Hide resolved
@wilfwilson wilfwilson added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions) labels Mar 23, 2021
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Let's wait until PR #4332 is merged then rebase this, and review again

@wilfwilson
Copy link
Member

Please rebase when you're ready @ChrisJefferson 🙂

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I took the liberty of rebasing this.

Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
@fingolfin
Copy link
Member

I've also applied @wilfwilson's patch (I hope you don't mind, Chris -- if you do, you know how to revert it)

@wilfwilson wilfwilson merged commit 5f48073 into gap-system:master Mar 26, 2021
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use 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 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library topic: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants