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

For groups: generic centralizer, subgroup methods; improving center #35540

Merged
merged 6 commits into from
May 22, 2023

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Apr 19, 2023

📚 Description

Only certain groups (mainly permutation groups) implement centralizer() and subgroups() methods, but GAP provides this functionality for more general groups. We provide this implementation for all finite groups implemented via GAP, which seems to do a brute-force approach. Likewise, the center() method for infinite groups seems to run forever, and we protect against that. We add an explicit center() method for Heisenberg matrix groups (which have a GAP group) both to demonstrate that specific (infinite) groups can implement it and to provide this information.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

dwbump pushed a commit to dwbump/sage that referenced this pull request Apr 19, 2023
@dwbump
Copy link
Contributor

dwbump commented Apr 20, 2023

I ran this combined with #35387, and can confirm it works well with the DrinfeldDouble method, which exercises conjugacy_classes_representatives and centralizer methods that were missing from matrix groups, at least for GL.

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 20, 2023

@dwbump Does that constitute a positive review or do you want someone else (say, who works more in the group code) to do the review of this PR?

@dwbump
Copy link
Contributor

dwbump commented Apr 21, 2023

Do any GAP experts have comments on this PR? @dimpase @fingolfin @ChrisJefferson

@dwbump
Copy link
Contributor

dwbump commented Apr 21, 2023

Analysis:

MatrixGroups are missing methods that by contrast are implemented for PermutationGroups. These include centralizer, subgroups, conjugacy_classes_subgroups and exponent methods.

The MatrixGroup class inherits from GroupMixinLibGAP class, so adding methods to the latter class adds them to MatrixGroup's. Other classes that inherit from GroupMixinLibGAP are FinitelyPresentedGroup, GroupLibGAP and AbelianGroup_gap. (The class PermutationGroup does not inherit from GroupMixinLibGAP.)

Of these classes, I think the main application will be to MatrixGroups. However one may imagine other classes of groups that could inherit from GroupMixinLibGAP for which it will be beneficial to have these methods. For example (perhaps) PermutationGroup might inherit from this class and then (perhaps) some duplicate methods could be removed. However this would not be a simple change and is beyond the scope of this PR. Another example: gap.SmallGroup(n,k) returns a finite GAP polycyclic group for many or most values of n and k, and perhaps there should be a class for such groups.

The newly implemented centralizer of a MatrixGroup element is itself a Matrix group, and hence inherits the methods of GroupMixinLibGAP.

My testing:

I checked that the centralizer method produces a subgroup, which I assume is correctly computed since this is done by GAP, for a variety of matrix groups. I tested GL(2,5), SL(2,5), PGL(2,5), PSL(2,5), SO(4,7,1), SO(4,7,-1), SO(5,5) and Sp(4,7). The last group has order about 276 million. In every case the calculations were pretty fast. I also tested the new centralizer method for finite Heisenberg groups (Heisenberg groups are MatrixGroups so they also inherit from GroupMixinLibGAP.)

Here is a test that exercises the code:

sage: G = GL(3,2)
sage: all(G.order()==G.centralizer(x).order()*G.conjugacy_class(x).cardinality() for x in G)
True

This takes 249 ms on my machine. This G is the simple group of order 168.

In the above testing I only computed centralizers for the output of self.conjugacy_classes_representatives. But for a limited number of small MatrixGroups I also computed centralizers for all elements of the group. I computed every centralizer of every element of GL(3,2), the simple group of order 168.

Additionally a new the center method is implemented by the PR in the Heisenberg group class. Infinite Heisenberg groups must be over ZZ, because Heisenberg groups infinite fields such as CC are not implemented. This means that the center is always cyclic, and if Heisenberg methods over CC were implemented (which they are not) a different implementation of the center would therefore be needed. In my testing I found that the new center method works, as well as (for finite Heisenberg groups) the new centralizer method.

Recommendations:

The FusionDouble code, which works with this, also tests the code. I would give the code a positive review but I have a couple of minor recommendations.

(1) The variable centralizer defined at line 447 of libgap_mixin.py is badly named. How about calling it centralizer_gens?
(2) How about further implementing the group_id method for this class. The code can be copied from permgroup.py.

@dwbump dwbump self-requested a review April 22, 2023 14:16
@dwbump dwbump mentioned this pull request Apr 22, 2023
4 tasks
@tscrim
Copy link
Collaborator Author

tscrim commented Apr 25, 2023

(1) The variable centralizer defined at line 447 of libgap_mixin.py is badly named. How about calling it centralizer_gens?

I don't think it is badly named, I have changed it.

(2) How about further implementing the group_id method for this class. The code can be copied from permgroup.py.

I had done this then saw your #35547. I merged that in here.

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 112ca3f

@dwbump
Copy link
Contributor

dwbump commented Apr 25, 2023

The pycodestyle complaints are unrelated to this PR since the files mentioned are in rings.real_double.pyx and rings/real_mpfr.pyx.

Copy link
Contributor

@dwbump dwbump left a comment

Choose a reason for hiding this comment

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

All the issues with this PR that I am aware of have been addressed. The Lint bot reports problems in real_double.pyx and real_mpfr.pyx but these files are unrelated to this PR.

@mkoeppe mkoeppe removed this from the sage-10.0 milestone May 4, 2023
@tscrim
Copy link
Collaborator Author

tscrim commented May 8, 2023

Thank you.

@vbraun vbraun merged commit cab4a6f into sagemath:develop May 22, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 22, 2023
@tscrim tscrim deleted the groups/matrix_group_centralizer branch May 23, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants