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

Add doublecoset benchmark #2979

Merged

Conversation

olexandr-konovalov
Copy link
Member

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.

Copy link
Contributor

@hulpke hulpke left a 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.

benchmark/doublecoset/doublecoset2.tst Outdated Show resolved Hide resolved
gap> START_TEST("doublecoset.tst");
gap> g:=SimpleGroup("J2");;
gap> m:=MaximalSubgroupClassReps(g);;
gap> u:=First(m,x->Index(g,x)=10080);;
Copy link
Contributor

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.

@olexandr-konovalov
Copy link
Member Author

@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:

cd benchmark/$BENCHMARK
../../bin/gap.sh -o 1g test1.g
../../bin/gap.sh -o 1g test2.g
../../bin/gap.sh -o 2g test3.g 

and then what I see is

screen shot 2018-11-09 at 11 56 12

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.
@codecov
Copy link

codecov bot commented Nov 10, 2018

Codecov Report

Merging #2979 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2979      +/-   ##
==========================================
+ Coverage   85.21%   85.21%   +<.01%     
==========================================
  Files         110      110              
  Lines       56857    56857              
==========================================
+ Hits        48450    48453       +3     
+ Misses       8407     8404       -3
Impacted Files Coverage Δ
src/iostream.c 63.49% <0%> (+1.14%) ⬆️

@olexandr-konovalov
Copy link
Member Author

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 make teststandard which fails in Travis because of the memory demands of tst/testextra/doublecoset.tst.

@olexandr-konovalov olexandr-konovalov merged commit 34b67c6 into gap-system:master Nov 12, 2018
@olexandr-konovalov olexandr-konovalov deleted the doublecoset-benchmark branch November 12, 2018 14:41
@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests labels Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants