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

Teach FrattiniSubgroup methods to check for solvability #2041

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

fingolfin
Copy link
Member

The generic method and the method using radicals both benefit from
checking solvability, and then redispatching (if solvability was not
known before, thus preventing an endless loop).

The point is that for the former, we are generic anyway, anything may be
either trivial to compute or impossible, so we might as well check for
solvability, which shouldn't be harder than computing all maximal
subgroups (plus, the latter is impossible for huge elementary abelian
groups, while the former is trivial).

The method using radicals automatically computes whether the group is
solvable anyway, so it may just as well use this and redispatch in that
case.

Resolves #710

This PR is currently against the master branch, as I am not sure how it might affect stability. But of course I can easily retarget it, should that seem desirable.

The generic method and the method using radicals both benefit from
checking solvability, and then redispatching (if solvability was not
known before, thus preventing an endless loop).

The point is that for the former, we are generic anyway, anything may be
either trivial to compute or impossible, so we might as well check for
solvability, which shouldn't be harder than computing all maximal
subgroups (plus, the latter is impossible for huge elementary abelian
groups, while the former is trivial).

The method using radicals automatically computes whether the group is
solvable anyway, so it may just as well use this and redispatch in that
case.

Resolves gap-system#710
@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Dec 20, 2017
@fingolfin fingolfin requested a review from hulpke December 20, 2017 09:22
@codecov
Copy link

codecov bot commented Dec 20, 2017

Codecov Report

Merging #2041 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2041      +/-   ##
==========================================
+ Coverage   66.03%   66.03%   +<.01%     
==========================================
  Files         898      898              
  Lines      273398   273406       +8     
  Branches    12792    12763      -29     
==========================================
+ Hits       180528   180536       +8     
+ Misses      90039    90038       -1     
- Partials     2831     2832       +1
Impacted Files Coverage Δ
lib/grp.gi 86.16% <100%> (+0.09%) ⬆️
lib/maxsub.gi 63% <100%> (+0.26%) ⬆️
src/hpc/traverse.c 77.99% <0%> (-0.78%) ⬇️
src/hpc/threadapi.c 34.46% <0%> (-0.19%) ⬇️
src/hpc/thread.c 46.01% <0%> (ø) ⬆️
src/funcs.c 76.37% <0%> (+0.13%) ⬆️
src/stats.c 79.43% <0%> (+0.13%) ⬆️

@fingolfin fingolfin merged commit ee54ef5 into gap-system:master Dec 21, 2017
@fingolfin fingolfin deleted the mh/frattini branch December 21, 2017 10:03
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.0 milestone Jan 22, 2018
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018
@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Jan 29, 2018

Added to release notes for GAP 4.10 at https://github.com/gap-system/gap/wiki/GAP-4.10-release-notes

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: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants