-
Notifications
You must be signed in to change notification settings - Fork 163
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
Conversation
3c69153
to
339e892
Compare
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.
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.
339e892
to
a15af94
Compare
The "fix PlainListCopy" part of this PR is now #4332 (this PR contains that PR, because it's needed for tests to pass) |
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.
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.
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.
Let's wait until PR #4332 is merged then rebase this, and review again
Please rebase when you're ready @ChrisJefferson 🙂 |
a15af94
to
adf8fb2
Compare
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.
I took the liberty of rebasing this.
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
I've also applied @wilfwilson's patch (I hope you don't mind, Chris -- if you do, you know how to revert it) |
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.