-
Notifications
You must be signed in to change notification settings - Fork 161
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
Some small enhancements on Sylow and Hall subgroup computations #535
Conversation
ff0d7cf
to
33bff13
Compare
This seems sensible, though I did not review it in detail so far. But I can make the obvious standard request ... :-): Could we perhaps get some tests, at least for the new methods? |
Corrected some typos and added tst file. Profiling shows that almost all new lines run (actually, almost all lines relevant to Sylow or Hall subgroup computations run), except for the new HallSubgroupOp method for nilpotent groups in grp.gi:2835. The main reason is explained in #558. That is This is actually problematic with several pcgs methods, that sometimes they have insanely high ranks, and nilpotent group methods will have no chance to run. The same would happen with SylowSubgroup, as well, but there somehow I could still test it with a finitely presented example. Probably ranks are better chosen there? How should we remedy this? |
Well, did you actually check whether your method for nilpotent groups is faster in the case of groups that |
@fingolfin For And in particular, I tried for big nilpotent permutation groups, and the permgroup method is by way faster for PCore. The main reason for this is that For In summary: the `SylowSubgroups BTW, does it make sense to have any of these operations for infinite groups? In my opinion it does, but according to the manual it does not. If we keep ourselves to the manual, then these methods should only be installed to groups with the |
I don't know of a definition of Sylow and Hall subgroups for infinite groups, so I don't think they should be available for such groups (unless you know of a good definition? and a way to actually compute something with that definition ;-) I am not quite sure what you mean when you say "In summary: the |
@fingolfin Sorry, I will be more specific. For the definition, one could call a Sylow subgroup a maximal (possibly infinite) p-subgroup, i.e. a maximal subgroup such that all its elements are of p-power order. According to the wikipedia page, for nilpotent groups the torsion group is the direct product of the Sylow subgroups, i.e. for nilpotent groups the method in lib/grp.gi:2774 would work with a slight modification (throw out generators with order infinity). I have not checked whether the wiki page is really correct, but I would if you (or anyone else for that matter) thinks that SylowSubgroups should be defined for infinite groups, as well. https://en.wikipedia.org/wiki/Nilpotent_group About ranking: SylowSubgroupOp: HallSubgroupOp: |
There are several (conflicting) definitions for Sylow subgroups of infinite groups. It's probably not a good idea to pick one more or less at random. In the abelian and nilpotent cases, it would be more common to call them p-torsion subgroups. Btw., throwing out infinite generators isn't enough: you can generate any infinite abelian group using only generators of infinite order – even in that case, you'd have to use IndependentGenerators. For nilpotent groups, you'll have to compute the torsion subgroup first. I guess it's better to ignore the infinite cases for the moment. |
What @bh11 was exactly my thought: I am aware of definitions, but I am not aware of a "good" definition -- the fact that multiple, conflicting definitions exist means that probably none of them is "good" (unless one is very, very widely used in the literature, and in some mathematical sense "better" than the others -- I am pretty sure that's not the case here). In general, I think an important key principle is to only write code that you actually use -- because if you write code (and that includes introducing code for sylow subgroups of infinite groups) which you do not actually use yourself, actively, then most likely you got it wrong anyway ;-). |
Very well, I added the IsFinite filter to the nilpotent methods for I have not touched the 100 rank increase in lib/grppcatr.gi:303, because I am not sure why this was introduced in the first place. Maybe somebody can comment on this? In grpprmcs.gi:1361 I have replaced the initial factorization of p and replaced it by an |
@hungaborhorvath Since this PR now merged, subject to passing more intensive tests, could you please add to the Wiki page for the next major release the text to include in the changes manual and the release announcement? |
@alex-konovalov I wrote something, it that ok? |
@hungaborhorvath yes, please only references to pull requests or issues in the same style like this is done in https://github.com/gap-system/gap/wiki/Changes-between-GAP-4.7-and-GAP-4.8. This will help a lot. I will not ask/reply to other PRs from the current batch, but please assume that this is the default procedure to document changes for the next release - it's easier to write this text as it goes than to delay it till the release time. Thanks! |
@alex-konovalov Ok, I added all PR numbers, links, and the PR titles. I did, however, change some of the PR titles on this wiki page (e.g. removed finite, where the method does in fact work for infinite groups), and could not make myself remove the explaining sentences. If you or anyone else does not like them, feel free to remove them. I will add explanations for the other PRs as they are merged. Thank you for pointing this out. |
Very good, thanks. There is no plan to keep the PR titles there (if you will scroll the page for GAP 4.8 until releases that happened already, you will see more polished texts), but I've put them there to ease understanding (and used italics to distinguish a temporary text from the rest). Text looks good, and with Wiki it's easy now to polish it if anybody would like. |
@alex-konovalov Ok, I italicized my text, and someone else can finalize it. |
Thanks. I've added a note to GAP 4.8 overview saying "THIS SECTION IS IN PROGRESS - PLEASE USE THE SAME STYLE AS IN THE RECENT RELEASES, LISTED BELOW" to avoid confusions:) |
This PR is supposed to address #412 and #511
Added PCore and HallSubgroup methods for nilpotent groups.
Used SetHallSubgroup and SetSylowSubgroup to store computed Hall subgroups and Sylow subgroups.
About the nilpotent group method, I am rather unsatisfied at the moment, because the pcgs methods are currently ranked higher. If the method would be installed for finite nilpotent groups, then it would rank higher. Should an extra method for finite nilpotent groups be added, or maybe should the nilpotent method be artificially ranked higher?