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 new method for IsSolvableGroup #3278

Merged
merged 6 commits into from Mar 22, 2019

Conversation

hungaborhorvath
Copy link
Contributor

@hungaborhorvath hungaborhorvath commented Feb 10, 2019

For checking solvability, if group knows its size, then it should check for the Feit-Thompson and for the Burnside criteria first.
The Feit-Thompson check is already implemented as an ImmediateMethod.

Was thinking of adding it as ImmediateMethod, but IsPrimePowerInt can be more resource hungry than most of the immediate methods. Maybe the 2-group check could be implemented, though, as an immediate method?

Kind of surprised the Burnside theorem was never implemented before.

For checking solvability, if group can compute its size, then it should, and
then should check for the Feit-Thompson and for the Burnside criteria first.
The Feit-Thompson check is already implemented as an ImmediateMethod.
@codecov
Copy link

codecov bot commented Feb 10, 2019

Codecov Report

Merging #3278 into master will increase coverage by 0.24%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #3278      +/-   ##
==========================================
+ Coverage   84.98%   85.22%   +0.24%     
==========================================
  Files         694      696       +2     
  Lines      343737   344065     +328     
==========================================
+ Hits       292124   293231    +1107     
+ Misses      51613    50834     -779
Impacted Files Coverage Δ
lib/morpheus.gd 100% <ø> (ø) ⬆️
lib/ctbl.gd 99.34% <ø> (ø) ⬆️
lib/teaching.g 88.07% <ø> (ø) ⬆️
lib/grp.gi 89.23% <95.45%> (+0.03%) ⬆️
lib/grpprmcs.gi 83.42% <0%> (-1.65%) ⬇️
src/weakptr.c 97.91% <0%> (-1.19%) ⬇️
lib/autsr.gi 34.22% <0%> (-1.13%) ⬇️
lib/clashom.gi 81.1% <0%> (-0.16%) ⬇️
src/hpc/threadapi.c 46.8% <0%> (-0.09%) ⬇️
lib/stbcbckt.gi 86.5% <0%> (-0.05%) ⬇️
... and 30 more

@hungaborhorvath
Copy link
Contributor Author

testmanuals fail for whatever reason. Anyone has any suggestions how I can fix it?

@fingolfin
Copy link
Member

For the failing test, the manual examples will have to be adjusted. Looking at https://api.travis-ci.org/v3/job/491232815/log.txt, you can search for "but found". It will tell you above that were the mismatch occurred.

@hungaborhorvath
Copy link
Contributor Author

hungaborhorvath commented Feb 11, 2019

@fingolfin I adjusted the new tests. I checked with Tracemethods that it runs the new method, anyway.

I saw that there are a lot of similar (HasIsAbelian, etc.) tests in the old version of IsSolvableGroup.tst. Should I rewrite those, as well?

As for the manuals, I am really puzzled what these couple lines managed to mess up. I checked what you suggested, there were 5 problems. 1 in the tutorial, 2 about homomorphisms, 2 about TransformingPermutationsCharacterTables. Are we sure that they were running with no error before I merged my changes? In any case, the examples there are rather fragile.

Ok, I think I fixed 4 out of 5 of the manual breaks. However, I cannot find where the source for the tutorial chapter 5 is...
Edit: Never mind, found it, hopefully fixed it, too.

@hulpke
Copy link
Contributor

hulpke commented Feb 12, 2019

Is this actually addressing any concrete problem (that is, are there examples of groups that can determine the size cheaply, but for which the solvability test is more expensive), or is this just a tehoretical addition for hypothetical cases?

At least for permutation groups and matrix groups (and thus also for FpGroups) I expect that testing solvability is cheaper than determining the size (and will determine the size on the way), using e.g. Sims' solvable Stabilizer chain algroithm, respectively composition tree. Thus I'm not sure what this method actually achieves.

(Also, typically one will want not just a statement that a group is solvable, but actually a corresponding data structure, e.g. a Pcgs.)

@hungaborhorvath
Copy link
Contributor Author

@hulpke The idea came when the other day someone asked on the forum about the normal subgroups of the unit group of F_4D_8. For me the solvability finished in more than 90 seconds (checked only for due diligence, at the time I wasn't willing to wait for it), but size could have been determined instantenously, and therefore it was clear that the group is solvable. (It was even nilpotent, as @Stefan-Kohl determined). I have to admit, though, that CanComputeSize was false for that group, even though determining size took only 4 ms.

Would you prefer if I replaced CanComputeSize with HasSize?

@hungaborhorvath
Copy link
Contributor Author

Can someone help me what I need to do about the failing testinstall suite?

@ChrisJefferson
Copy link
Contributor

The issue with the 'JULIA=yes' test you can ignore, it's not a problem you have caused.

@coveralls
Copy link

coveralls commented Feb 12, 2019

Coverage Status

Coverage remained the same at 85.061% when pulling bd8b613 on hungaborhorvath:SolvableByNoPrimes into efcb6c5 on gap-system:master.

@hulpke
Copy link
Contributor

hulpke commented Feb 12, 2019

@hungaborhorvath Using HasSize would at least avoid serious performance problems. (E.g. for certain larger nonsolvable permutation groups where a quick nonsolvability test is desired before calculating the size.) Thus I would be strongly in favor of such a change. Note that this might revert some of the examples that changed.

CanComputeSize is just about capability to do so -- e.g. a finitely resenetd group would not know it a priori.

(I also think it is not good policy to use very much nonstandard examples -- such as this group of units in F4D8, which cannot do anything but element arithmetic -- to design general functionality.)

@hungaborhorvath
Copy link
Contributor Author

@hulpke Replaced CanComputeSize by HasSize.

hulpke
hulpke previously requested changes Feb 12, 2019
lib/grp.gi Show resolved Hide resolved
lib/grp.gi Show resolved Hide resolved
@hulpke hulpke dismissed their stale review February 12, 2019 23:21

Author remarks

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.

Fine by me, although I think it's usefulness in practice is fairly limited.

@ChrisJefferson ChrisJefferson merged commit 9e7e38d into gap-system:master Mar 22, 2019
@hungaborhorvath hungaborhorvath deleted the SolvableByNoPrimes branch March 22, 2019 20:54
@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Apr 15, 2019
@fingolfin fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: library labels Apr 15, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: added PRs introducing changes that have since been mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
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.

None yet

7 participants