-
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
Add doublecoset benchmark #2979
Add doublecoset benchmark #2979
Conversation
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 approve as I do not like the idea of blocking merging from system authors until a PR has been approved.
However the two extra examples used do not contribute meaningfully to a test of the new features.
gap> START_TEST("doublecoset.tst"); | ||
gap> g:=SimpleGroup("J2");; | ||
gap> m:=MaximalSubgroupClassReps(g);; | ||
gap> u:=First(m,x->Index(g,x)=10080);; |
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 isn't really a benchmark for double cosets, as the index is small enough to even calculate orbits on cosets. (In fact the whole calculation on a group of order barely 600k is a very easy case)
The issue that the new code addresses is with subgroups of large index that do not have satisfactory intermediate subgroups (which is a given if they are maximal), so that a Leiterspiel with up-step is required.
One could try to have larger examples without Leiterspiel (just requiring to find intermediate subgroups), but as long as indices are not in the millions this is all childs play.
@hulpke Thanks - I will adjust the names of the tests before merging this. The reason to have three tests is that all other benchmarks also have three tests, and it's easy to have a common script in Jenkins which is parametrised by the name of the directory:
and then what I see is The idea is that the three tests are of increasing difficulty. If you could contribute two examples, one faster than Co3, and another either faster or slower, I will be happy to use them as more meaningful! |
The test from tst/testextra/doublecoset.tst requires more memory than in the default `make teststandard` setting, so it is moved to benchmarks, with two more shorter tests added.
d0b499c
to
1c05832
Compare
Codecov Report
@@ Coverage Diff @@
## master #2979 +/- ##
==========================================
+ Coverage 85.21% 85.21% +<.01%
==========================================
Files 110 110
Lines 56857 56857
==========================================
+ Hits 48450 48453 +3
+ Misses 8407 8404 -3
|
I've updated test files - now I'd rather merge this and changed examples in benchmarks later, to get it out of the way of |
The test from tst/testextra/doublecoset.tst, added in #2741 by @hulpke, requires more memory than in the default
make teststandard
setting, so it is moved to benchmarks, with two more shorter tests added.