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

Performance improvements #3451

Merged
merged 6 commits into from Jun 4, 2019
Merged

Performance improvements #3451

merged 6 commits into from Jun 4, 2019

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented May 14, 2019

Description

Improvements that mostly relate to computing double cosets, in particular in symmetric groups, and to permutation representations of extensions:

  • Conjugation of subgroups into other subgroup uses permutation group orbits first
  • coding improvements in double coset routine
  • Try to use good presentation if group is Sn/An. Fix for wrong assumption in complement code this throwed up.
  • RepresentativeAction method for OnTuplesSets
  • Coding improvement in finding permrep. for extensions.
  • Resolve Errors in testextra/grpperm.tst in master branch #3464

Text for release notes

Performance of double coset calculations, in particular in symmetric and alternating groups, has been improved.

@hulpke hulpke added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes do not comment yet PRs on which the author does not yet want any comment (e.g. only submitted for test results) labels May 14, 2019
@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #3451 into master will increase coverage by 0.04%.
The diff coverage is 79.17%.

@@            Coverage Diff             @@
##           master    #3451      +/-   ##
==========================================
+ Coverage   85.34%   85.38%   +0.04%     
==========================================
  Files         699      699              
  Lines      346504   346767     +263     
==========================================
+ Hits       295733   296099     +366     
+ Misses      50771    50668     -103
Impacted Files Coverage Δ
lib/grp.gd 100% <ø> (ø) ⬆️
lib/schur.gd 100% <ø> (ø) ⬆️
lib/twocohom.gd 100% <ø> (ø) ⬆️
lib/ghomfp.gi 87.04% <ø> (+0.58%) ⬆️
lib/gpprmsya.gi 85.82% <100%> (+7.52%) ⬆️
lib/gpfpiso.gi 66.71% <100%> (+0.2%) ⬆️
lib/grpperm.gi 90.84% <100%> (ø) ⬆️
lib/oprtperm.gi 93.75% <100%> (+0.49%) ⬆️
lib/onecohom.gi 65.77% <100%> (ø) ⬆️
lib/factgrp.gi 84.71% <100%> (+0.87%) ⬆️
... and 34 more

@hulpke hulpke removed the do not comment yet PRs on which the author does not yet want any comment (e.g. only submitted for test results) label May 16, 2019
@coveralls
Copy link

coveralls commented May 17, 2019

Coverage Status

Coverage increased (+0.04%) to 85.211% when pulling 2eed4f6 on hulpke:additions into 5fbcf2b on gap-system:master.

@hulpke hulpke removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label May 17, 2019
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.

Some minor remarks

lib/gpprmsya.gi Show resolved Hide resolved
lib/gpprmsya.gi Outdated Show resolved Hide resolved
lib/gpprmsya.gi Outdated Show resolved Hide resolved
lib/csetgrp.gi Show resolved Hide resolved
lib/csetgrp.gi Outdated Show resolved Hide resolved
lib/csetgrp.gi Show resolved Hide resolved
lib/csetgrp.gi Outdated Show resolved Hide resolved
lib/csetgrp.gi Show resolved Hide resolved
Also RepresentativeAction for OnTuplesSets of points.
- ContainedConjugates uses perm. group orbits
- Double cosets considers subgroups by increasing index
- Move (same) calculation outside inner loop

Fix assumption about generators of fphom

Code wrongly assumed that isomorphism fo fp was always to free generators.
This is not guaranteed (but so far always held, so was not spotted).
Use representation theory to predict which subgroup, w/o initial rewriting.
Directly write down permrep for semidirect product.

Moved twocohom.tst to teststandard, as it seems to be better place
to reduce load on travis

Added utility function to iterate through subgroups.
Increasing index, initially don't attempt to compute whole lattice.
Not in manual as it could make conjugates, ordering not perfect
Avoid infinite loop in bad cases of NHBNS.
Allow slightly more generators in initial check.
Test for running, n ot concrete output. Also change one assertion level
higher so that it is not triggered by the (arbitrarily chosen level of) the
automatic tests.
(This is safer than ad-hoc quickly changing complicated but stable code.)
This is a "be nice" assertion (that was never intended for automated tests),
not a must have.

This resolves gap-system#3464
@olexandr-konovalov
Copy link
Member

REMINDER - to all PR authors and reviewers: if a PR adds tests to tst/testextra, do not assume that it's tested with Travis CI, for it is not. Please test it manually (this PR added a broken test to tst/testextra, which had been fixed in #3501).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors in testextra/grpperm.tst in master branch
4 participants