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 property IsRegularPGroup #5359

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

fingolfin
Copy link
Member

Unfortunately I am not aware of an efficient algorithm for this in general. But the provided one at least performs reasonably for e.g. the groups of order 3^7 or 5^7.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature topic: library release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Feb 1, 2023
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.
I have left a few minor comments.

Just for curiosity:
Why is Agemo not an attribute?
(Agemo is defined only for p-groups, why is it necessary to enter p as an argument?)

tst/testinstall/pgroups.tst Outdated Show resolved Hide resolved
lib/grp.gd Outdated Show resolved Hide resolved
lib/grp.gi Outdated Show resolved Hide resolved
lib/grp.gi Outdated Show resolved Hide resolved
Unfortunately I am not aware of an efficient algorithm for this
in general. But the provided one at least performs reasonably for e.g.
the groups of order 3^7.
@fingolfin
Copy link
Member Author

Agemo is not an attribute because Agemo takes 2 or 3 arguments, so it can't be an attribute. Instead, the computed values are cached in mutable attribute ComputedAgemos.

I am also puzzled why Agemo takes the prime as argument, given that p = PrimePGroup(G) must always hold (and this is enforced in Agemo). This also conflicts with the notation in the literature... But I guess we are stuck with it now.

@fingolfin
Copy link
Member Author

Thanks for the careful checking and excellent feedback!

@ThomasBreuer ThomasBreuer merged commit fdc3aef into gap-system:master Feb 4, 2023
@jackschmidt
Copy link
Contributor

I wrote a few more tests to improve test coverage. I didn't find any group where the last special case code applied, but the other cases all have "smallest" examples. The cases are numbered (I added debug prints to each if statement), and all but case 11 (use the definition, and get true as the answer) are very quick.

Add the following (except maybe the last two lines, case 11) to tst/testinstall/pgroups.tst right near the end

gap> IsRegularPGroup(SmallGroup(8, 3)); # Case 1
false
gap> IsRegularPGroup(SmallGroup(2187, 224)); # Case 2
false
gap> IsRegularPGroup(SmallGroup(27,3)); # Case 3
true
gap> IsRegularPGroup(SmallGroup(81, 1)); # Case 4
true
gap> IsRegularPGroup(SmallGroup(243,22)); # Case 5
true
gap> IsRegularPGroup(SmallGroup(2187, 4487)); # Case 6
true
gap> IsRegularPGroup(SmallGroup(81, 7)); # Case 7
false
gap> IsRegularPGroup(SmallGroup(78125, 684)); # Case 8
true
gap> IsRegularPGroup(SmallGroup(243, 51)); # Case 10 (no case 9 found)
false
gap> IsRegularPGroup(SmallGroup(2187, 663)); # Case 11, takes 3 seconds
true

The following would be a nice case to record, but it takes over a minute to run, so I'm not sure it is worth including.

gap> G := SmallGroup(243,22);;
gap> IsRegularPGroup(G); # Case 5
true 
gap> IsRegularPGroup(DirectProduct(G,G)); # Hup 10.3.c, Wielandt's example
false

I've run out of time myself. I wanted to find a case 9, and submit a pull request, or improve the speed on Wielandt's example, but perhaps this is enough.

@jackschmidt
Copy link
Contributor

Oh, and thank you for adding such a nice check for regular p-groups :-) I'm also happy about the nice powerful p-group check above as well!

@fingolfin fingolfin deleted the mh/IsRegularPGroup branch February 7, 2023 10:47
fingolfin added a commit to fingolfin/gap that referenced this pull request Aug 20, 2024
Contributed by Jack Schmidt in a comment on PR gap-system#5359.

Also change phrasing in the documentation for IsRegularPGroup.
fingolfin added a commit that referenced this pull request Aug 27, 2024
Contributed by Jack Schmidt in a comment on PR #5359.

Also change phrasing in the documentation for IsRegularPGroup.
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 kind: new feature release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants